-
Notifications
You must be signed in to change notification settings - Fork 147
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
modules/btrfs: add GetDefaultSubvolumeID #1222
modules/btrfs: add GetDefaultSubvolumeID #1222
Conversation
<!-- | ||
GetDefaultSubvolumeID: | ||
@options: Additional options. | ||
@since: 2.11.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: not sure if this is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version number? Yes, that's correct.
Can one of the admins verify this patch? |
Jenkins, ok to test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you.
src/tests/dbus-tests/test_btrfs.py
Outdated
fstype = self.get_property(dev.obj, '.Block', 'IdType') | ||
fstype.assertEqual('btrfs') | ||
|
||
default_id = dev.obj.GetDefaultSubvolumeID(self.no_options, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the _temp_mount
context manager to mount the volume for this operation, the tests are now failing because the device is not mounted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
As long as getting the value requires a separate libblockdev call and perhaps some I/O, I'd suggest to leave it as a method, rather than a property.
libblockdev provides a method to get the default subvolume id since 2014, with no way to change it. This method does not require any administrative privileges but requires it for setting.
142202c
to
c1c6adb
Compare
Right, it would indeed be a bit wasteful and it's fine for us if it's a method 👍 |
Well, theoretically, since this is a plugin that needs to be activated first, any expensive I/O done on every uevent for btrfs devices may not be that much of a problem. At the end it depends on how often would you read the value. |
libblockdev provides a method to get the default subvolume id since 2014, with no way to change it. This method does not require any administrative privileges but requires it for setting.
This feels more logical as a property but it requires a method call and I intend to also add a Setter for this. But that requires a libblockdev patch.
P.S. I hope the tests succeed. I'm unable to get the tests running on Arch Linux as it uses
lsb_release
andtargetcli
. Targetcli is not available as a package.