Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow link argument to AddrAdd, AddReplace and AddrDel to be nil #948

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

cwmos
Copy link

@cwmos cwmos commented Jan 31, 2024

This to address #947

@jellonek
Copy link
Contributor

lgtm

@jellonek
Copy link
Contributor

@aboch, @vishvananda can you take a look? imo this change is clean and simple.

@aboch
Copy link
Collaborator

aboch commented Feb 24, 2024

Thank you @cwmos and @jellonek
Would it be possible to add a UT function for this?
Programming and then reading from kernel an address where the link parameter is nil, if not there yet.

@jellonek
Copy link
Contributor

imo it is in the same bucket as checking if user provided non nil addr. we are not checking that what leads to panic, because developer calling this function did a horrible mistake.
UT would be nice to have, but imo is not needed. (i would merge that on your place :P )

@aboch
Copy link
Collaborator

aboch commented Feb 25, 2024

If you do not want to add the ut to cover the following two lines

	if link == nil {
		msg.Index = uint32(addr.LinkIndex)

then I have to do it myself.
Believe me, it is going to take longer than one of you guys doing it.
Please add it.

@cwmos
Copy link
Author

cwmos commented Feb 25, 2024

I agree a unit test is in place here - I don't want this use case to break in the future :-)

I will try to add one.

@aboch
Copy link
Collaborator

aboch commented Mar 1, 2024

Thank you @cwmos
Please squash your two commits into one.
This project follows a "one commit per feature/fix/enhancement" approach

@cwmos
Copy link
Author

cwmos commented Mar 4, 2024

Done @aboch

@aboch
Copy link
Collaborator

aboch commented Mar 4, 2024

LGTM

@aboch aboch merged commit 70def89 into vishvananda:main Mar 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants