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 netlink with no owner/group #835

Closed
wants to merge 1 commit into from

Conversation

mmirecki
Copy link

@mmirecki mmirecki commented Jan 9, 2023

Currently it is only possible to create a tuntap with owner/group specified. If not set explicitly, the default value of 0 will be used. It's therefore not possible to create a tuntap without owner/group. This PR changes this to allow this. It changes the type of owner/group from uint to int to allow setting owner/group to -1, indicating no owner/group.

Currently it is only possible to create a tuntap with owner/group specified.
If not set explicitly, the default value of 0 will be used. It's therefore
not possible to create a tuntap without owner/group.
This PR changes this to allow this. It changes the type of owner/group from
uint to int to allow setting owner/group to -1, indicating no owner/group.
@mmirecki
Copy link
Author

@vishvananda @aboch Could you please take a look at this? Thanks

@dcbw
Copy link
Contributor

dcbw commented Jan 16, 2023

looks good to me, though it would be a downstream code change if for some reason consumers relied on the uint64 type

@mmirecki
Copy link
Author

looks good to me, though it would be a downstream code change if for some reason consumers relied on the uint64 type

Unfortunately we need to change the api one way or another to pass information about "no value".
The other options are:

  • use *uint with nil for no owner/group - but we don't use pointers in the api so it could be confusing
  • add new attributes hasOwner/hasGroup, this would be backwards compatible, but would add redundant parameters

@dcbw
Copy link
Contributor

dcbw commented Feb 6, 2023

Yeah, probably OK since it's a fairly minor change and adds a useful benefit.

LGTM

@aboch
Copy link
Collaborator

aboch commented Feb 6, 2023

[...] it would be a downstream code change if for some reason consumers relied on the uint64 type

Yes, that is the problem

@mmirecki
Copy link
Author

[...] it would be a downstream code change if for some reason consumers relied on the uint64 type

Yes, that is the problem

Fixing the problem will need to break the backwards compatibility in some way (except if we add new parameters to indicated the condition, which would probably be even uglier)
Any suggestions on which way of breaking it would be the least evil?

@mmirecki mmirecki closed this Jun 13, 2023
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.

None yet

3 participants