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

udiskslinuxdrive: Mark external NVMe removable #1234

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

khfeng
Copy link
Contributor

@khfeng khfeng commented Dec 20, 2023

Thunderbolt ports propagate "removable" attribute to its child devices like external NVMe, so use the attribute to set removable accordingly.

Excerpt from udevadm info -a /dev/nvme1n1:
looking at parent device
'/devices/pci0000:00/0000:00:1c.0/0000:03:00.0/0000:04:04.0/0000:3a:00.0/0000:3b:01.0/0000:3c:00.0':
KERNELS=="0000:3c:00.0"
SUBSYSTEMS=="pci"
DRIVERS=="nvme"
...
ATTRS{removable}=="removable"

#793

@StorageGhoul
Copy link

Can one of the admins verify this patch?

@tbzatek
Copy link
Member

tbzatek commented Dec 28, 2023

Jenkins, ok to test.

Copy link
Member

@tbzatek tbzatek left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. Could you also fix the code style? Check the surrounding code for guidance and avoid using tabs.

GUdevDevice *d;
const gchar **removable;

removable = g_udev_device_get_sysfs_attr_as_strv (parent, "removable");
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to use the construct you've moved further down in the patch? I.e. use g_udev_device_get_sysfs_attr_as_boolean() which tests the presence or an empty file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's a string instead of a boolean, i.e. ATTRS{removable}=="removable".

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but look at the line 517 in your patch - it's an old code that retrieves the value as boolean. I guess it's just a trick treating non-empty string as TRUE and it was working for ages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's really boolean. The sysfs attribute is created based on GENHD_FL_REMOVABLE flag. CD-ROM or USB drive have this.

OTOH, the NVMe external drive's removable is created based on DEVICE_REMOVABLE and it's a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So is it enough to get this merged?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I see now. Didn't expect that.

Thunderbolt ports propagate "removable" attribute to its child devices
like external NVMe, so use the attribute to set removable accordingly.

Excerpt from `udevadm info -a /dev/nvme1n1`:
  looking at parent device
'/devices/pci0000:00/0000:00:1c.0/0000:03:00.0/0000:04:04.0/0000:3a:00.0/0000:3b:01.0/0000:3c:00.0':
    KERNELS=="0000:3c:00.0"
    SUBSYSTEMS=="pci"
    DRIVERS=="nvme"
...
    ATTRS{removable}=="removable"

storaged-project#793
Copy link

Cockpit tests failed for commit c362d98. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

martinpitt commented Jan 4, 2024

Sorry, not your fault. We reported that lvm failure to https://bugzilla.redhat.com/show_bug.cgi?id=2256433 and will disable the test in rawhide, as it causes an unsalvageable mess. Please ignore.

Update: this happened, so if you force-push or retry the test it should work.

Copy link
Member

@tbzatek tbzatek left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@tbzatek tbzatek merged commit 77b0afa into storaged-project:master Jan 22, 2024
17 of 18 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.

4 participants