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.

Sign up for GitHub

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

Jump to bottom

Device: Add ownership.inherit setting for unix-hotplug devices #14417

Merged
merged 7 commits into from
Dec 3, 2024
Merged

Device: Add ownership.inherit setting for unix-hotplug devices #14417

merged 7 commits into from
Dec 3, 2024

Conversation

kadinsayani
Copy link
Contributor

@kadinsayani kadinsayani commented Nov 7, 2024 •
edited
Loading

Fixes the second bug raised in #14266.

This PR adds a setting, ownership.inherit for unix-hotplug devices and a function unixDeviceOwnership() which returns device ownership.

New behaviour:

  • The default setting for ownership.inherit is false;
  • When ownership.inherit is set to true, the device ownership is inherited from the host;
  • ownership.inherit cannot be set to true when gid and uid are set.

Existing behaviour (unchanged):

  • When gid and uid are set, they are used for device ownership;
  • If gid and uid are not present in the config, root (0) ownership is used.

@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch from a0b63fd to 8fed8a3 Compare November 7, 2024 23:07
@kadinsayani kadinsayani changed the title unix-hotplug ownership fix lxd/device: unix-hotplug ownership fix Nov 7, 2024
@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch from 8fed8a3 to 0d5c055 Compare November 7, 2024 23:20
simondeziel
simondeziel previously approved these changes Nov 8, 2024
View reviewed changes
@tomponline
Copy link
Member

@kadinsayani please can you update the PR description to explain the change of behaviour in this PR and what, if any, will change for existing users?

@kadinsayani
Copy link
Contributor Author

@kadinsayani please can you update the PR description to explain the change of behaviour in this PR and what, if any, will change for existing users?

Sure thing!

@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch 3 times, most recently from db02e22 to 4c9ee63 Compare November 12, 2024 16:08
@github-actions github-actions bot added the Documentation Documentation needs updating label Nov 12, 2024
@github-actions GitHub Actions
Copy link

Heads up @mionaalex - the "Documentation" label was applied to this issue.

@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch 3 times, most recently from a25f26f to 81478b0 Compare November 12, 2024 18:37
@kadinsayani
Copy link
Contributor Author

I noticed some wording inconsistencies in the documentation for unix-hotplug and the other unix device types. I've opened #14451 to track the issue and engage @minaelee for feedback. I'll open up a separate PR to update the docs and resolve the issue.

simondeziel
simondeziel previously approved these changes Nov 13, 2024
View reviewed changes
@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch 2 times, most recently from 13be986 to ebc953a Compare November 13, 2024 17:19
@kadinsayani
Copy link
Contributor Author

@tomponline, do we need an API extension for the new setting even if we're not adding tests?

@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch from ebc953a to c78aeda Compare November 13, 2024 17:43
@tomponline
Copy link
Member

@tomponline, do we need an API extension for the new setting even if we're not adding tests?

yes new settings require API extensions in order for clients to be able to ascertain whether the server supports the setting.

@kadinsayani
Copy link
Contributor Author

@tomponline, do we need an API extension for the new setting even if we're not adding tests?

yes new settings require API extensions in order for clients to be able to ascertain whether the server supports the setting.

Gotcha, makes sense.

@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch from c78aeda to 2f191aa Compare November 14, 2024 15:46
@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch 5 times, most recently from 8aa9181 to b50e48b Compare November 18, 2024 18:28
@kadinsayani kadinsayani mentioned this pull request Nov 18, 2024
Closed
@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch from c3562a4 to c6ab941 Compare November 18, 2024 23:05
@kadinsayani kadinsayani changed the title lxd/device: unix-hotplug ownership fix lxd/device: Add ownership.inherit setting for unix-hotplug devices Nov 19, 2024
@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch from c6ab941 to 41ef286 Compare November 25, 2024 19:30
@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch from 41ef286 to 98feba4 Compare November 25, 2024 19:42
@kadinsayani kadinsayani linked an issue Nov 25, 2024 that may be closed by this pull request
Inconsistency in unix-hotplug when multiple devices share product/vendor IDs #14266
Closed
@kadinsayani kadinsayani changed the title lxd/device: Add ownership.inherit setting for unix-hotplug devices Device: Add ownership.inherit setting for unix-hotplug devices Nov 26, 2024
@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch from 98feba4 to c143d04 Compare November 29, 2024 15:42
@kadinsayani
Copy link
Contributor Author

@tomponline rebased 👍

@tomponline
Copy link
Member

rebase please

@kadinsayani
lxd/device: Use device Clone() function rather than duplicate confi…
3462874 
…g copy logic

Signed-off-by: Kadin Sayani <[email protected]>
@kadinsayani
lxd/device: Fix comment typos
cd6a0ea 
Signed-off-by: Kadin Sayani <[email protected]>
@kadinsayani
lxd/device: Add named return results to unixDeviceAttributes
f535323 
Signed-off-by: Kadin Sayani <[email protected]>
@kadinsayani
api: Add unix_device_hotplug_ownership_inherit extension
6c2c08d 
Signed-off-by: Kadin Sayani <[email protected]>
@kadinsayani
lxd/device: Add ownership.inherit setting for unix-hotplug devices
2cbc524 
Signed-off-by: Kadin Sayani <[email protected]>
@kadinsayani
lxd/device: Use %q when quoting values in error messages where appl…
b505fa6 
…icable

Signed-off-by: Kadin Sayani <[email protected]>
@kadinsayani
metadata: Run make update-metadata
de25799 
Signed-off-by: Kadin Sayani <[email protected]>
@kadinsayani kadinsayani force-pushed the 14266-unix-hotplug-ownership-fix branch from c143d04 to de25799 Compare December 2, 2024 14:20
@kadinsayani
Copy link
Contributor Author

rebase please

Rebased.

tomponline
tomponline approved these changes Dec 3, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@tomponline tomponline merged commit cb1a3e0 into canonical:main Dec 3, 2024
27 checks passed
@kadinsayani kadinsayani deleted the 14266-unix-hotplug-ownership-fix branch December 3, 2024 14:55
@jonathan-conder jonathan-conder mentioned this pull request Dec 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simondeziel simondeziel simondeziel left review comments

@tomponline tomponline tomponline approved these changes

Assignees
No one assigned
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistency in unix-hotplug when multiple devices share product/vendor IDs
3 participants
@kadinsayani @tomponline @simondeziel