udiskslinuxmanager: add resolving devices by partuuid/partlabel #861
Conversation
|
Can one of the admins verify this patch? |
|
Jenkins, ok to test. |
There was a problem hiding this comment.
Lookup using both partuuid and partlabel is already possible with paths: both /dev/disk/by-partlabel/xxx and /dev/disk/by-partuuid/xxx should work. But I'm not against adding these two to make the API a little bit more user friendly.
Also using the paths internally would make the code way simpler (e.g. constructing the /dev/disk/by- path and using same helper function we use for devpath).
| @@ -1224,6 +1230,27 @@ handle_resolve_device (UDisksManager *object, | |||
| if (devlabel != NULL) | |||
| found = g_strcmp0 (udisks_block_get_id_label (blocks_p->data), devlabel) == 0; | |||
|
|
|||
| if (partlabel != NULL || partuuid != NULL) | |||
There was a problem hiding this comment.
These two checks should be separate -- you need to match both partlabel and partuuid when user specifies both (documentation for ResolveDevice allows specifying multiple keys and all must match).
There was a problem hiding this comment.
True, that OR between the tests doesn't conform to the "devices matching all values" condition.
Also, you should be pulling strings from the org.freedesktop.UDisks2.Partition interface of the block object as suggested in the D-Bus API docs addition. The udev ID_PART_ENTRY_ properties are encoded strings (e.g. ID_PART_ENTRY_NAME=EFI\x20System\x20Partition) and are different from what is presented on the .Partition interface.
There was a problem hiding this comment.
FWIW, this code is borrowed from https://github.com/storaged-project/udisks/blob/udisks-2.9.2/src/udiskslinuxblock.c#L424. But from what I can tell udisks_linux_block_matches_id has a different use case as we don't have "matching all values" situation there.
Ok, I'll try to use .Partition interface. Maybe should be fixed in udisks_linux_block_matches_id as well?
There was a problem hiding this comment.
Indeed, thanks for bringing this up. I've tried to addess this in #888.
|
|
||
| blocks = get_block_objects (object, &num_blocks); | ||
|
|
||
| for (blocks_p = blocks; blocks_p != NULL; blocks_p = blocks_p->next) | ||
| { | ||
| found = FALSE; |
There was a problem hiding this comment.
Good catch! Actually the logic around found is completely wrong here, overwriting the value as the checks go, making only the last requested check relevant. So much for the "only devices matching all values" statement.
Could you please correct all the checks while you're at it? (Alternatively we can deal with that later, this would need to be covered by test cases anyway).
There was a problem hiding this comment.
Oh I see. Yeah I added found = FALSE; since that would replicate what happened before, but yeah "only devices matching all values" is defunct currently. Will fix this.
| @@ -1224,6 +1230,27 @@ handle_resolve_device (UDisksManager *object, | |||
| if (devlabel != NULL) | |||
| found = g_strcmp0 (udisks_block_get_id_label (blocks_p->data), devlabel) == 0; | |||
|
|
|||
| if (partlabel != NULL || partuuid != NULL) | |||
There was a problem hiding this comment.
True, that OR between the tests doesn't conform to the "devices matching all values" condition.
Also, you should be pulling strings from the org.freedesktop.UDisks2.Partition interface of the block object as suggested in the D-Bus API docs addition. The udev ID_PART_ENTRY_ properties are encoded strings (e.g. ID_PART_ENTRY_NAME=EFI\x20System\x20Partition) and are different from what is presented on the .Partition interface.
|
I've incorporated the notes above and slightly refactored the code, resulting in #933. Closing this one. |
Allow to find block devices by partition name and partition UUID. Use strings similar to how Linux allows to find such block devices (
partuuidandpartlabel).