Created attachment 236697 [details] [review]
installer-media: Fetch original volume ID from udev

udev replaces spaces (' ') with underscores ('_') in 'ID_FS_LABEL'
property. Since there is no way to know if an underscore was
originally a space or not in the source volume ID, we better use
'ID_FS_LABEL_ENC' instead. In that property spaces are escaped by '\x20'
and hence we can be much more certain about it being originally a space.

Created attachment 236698 [details] [review]
os-database: Rename guess_os_from_install_media

Rename OSDatabase.guess_os_from_install_media method to
guess_os_from_install_media_from_path not only to better represent it
but also to make way for a new function for which this name fits better.

Created attachment 236699 [details] [review]
os-database: Add guess_os_from_install_media

A new method to guess media. Unlike guess_os_from_install_media_path,
this takes Osinfo.Media as input.

Created attachment 236700 [details] [review]
installer-media: Make use of new udev properties

New udev/blkid expose more ISO9660 properties. The libosinfo udev rules
that provided the 'OSINFO_*' properties will soon be ditched as a
result.

This patch adapts the code for these changes but without breaking it
against existing libosinfo/udev.

Review of attachment 236697 [details] [review]:

::: src/installer-media.vala
@@ +164,3 @@
+
+        // FIXME: Do we need to decode anything other than spaces?
+        return (encoded != null)? encoded.replace ("\\x20", " ") : null;

This can't be right, we should do a proper unescape rather than this lame thing. At the very least we need to handle e.g. \ being escaped.

Review of attachment 236698 [details] [review]:

ack

Review of attachment 236699 [details] [review]:

ack

Review of attachment 236700 [details] [review]:

ack

Review of attachment 236698 [details] [review]:

'guess_os_from_install_media_from_path' typo in the commit log (one extra 'from')

Review of attachment 236700 [details] [review]:

::: src/installer-media.vala
@@ +166,3 @@
+                                           { MEDIA_PROP_SYSTEM_ID, MEDIA_PROP_PUBLISHER_ID, MEDIA_PROP_APPLICATION_ID },
+                                           media);
+

Fwiw I'd go with something like 
var media = osinfo_media_new_from_udev(device, device_file);
(don't know the vala name I'd pick)
and just hardcode the name of the udev properties there.

Review of attachment 236697 [details] [review]:

::: src/installer-media.vala
@@ +164,3 @@
+
+        // FIXME: Do we need to decode anything other than spaces?
+        return (encoded != null)? encoded.replace ("\\x20", " ") : null;

'\' can't occur in the string as itself either so no escaping needed for that. This is the whitelisting function that blkid uses to decide whether a character needs to be replaced by '\xHEXVAL':

static int is_whitelisted(char c, const char *white)
{
   if ((c >= '0' && c <= '9') ||
       (c >= 'A' && c <= 'Z') ||
       (c >= 'a' && c <= 'z') ||
       strchr("#+-.:=@_", c) != NULL ||
       (white != NULL && strchr(white, c) != NULL))
      return 1;
   return 0;
}

where 'while' is passed as NULL in this case.

I think we just need to make the code more generic: i-e replace each '\xHEX' with the actual character.

Created attachment 236768 [details] [review]
installer-media: Fetch original volume ID from udev

v2: Improved decoding of udev values to deal with all encoded characters.

Review of attachment 236697 [details] [review]:

::: src/installer-media.vala
@@ +164,3 @@
+
+        // FIXME: Do we need to decode anything other than spaces?
+        return (encoded != null)? encoded.replace ("\\x20", " ") : null;

Yeah, but the actual encoder code does:

if (str[i] == '\\' || !is_whitelisted(str[i], NULL)) {
                        if (len-j < 4)
                                goto err;
                        sprintf(&str_enc[j], "\\x%02x", (unsigned char) str[i]);

i.e. it most definately does escape \, as how else would you reliably unescape such an escaped string???

Review of attachment 236768 [details] [review]:

::: src/installer-media.vala
@@ +147,3 @@
             throw new OSDatabaseError.NON_BOOTABLE ("Media %s is not bootable.", device_file);
 
+        label = get_udev_property (device, "ID_FS_LABEL_ENC");

I think this is much more naturally writtten as:
label = udev_decode_string (device.get_property ("ID_FS_LABEL_ENC"))
rather than moving the property getter into a method which is not at all naturally named (i.e. you have no idea how it differs from device.get_property(), or that it unescapes the string).

@@ +167,3 @@
+           uint8 x;
+
+           if (encoded[i:encoded.length].scanf ("\\x%02x", out x) > 0) {

It seems somewhat weird to decode using scanf, but i guess that works.

Review of attachment 236768 [details] [review]:

::: src/installer-media.vala
@@ +147,3 @@
             throw new OSDatabaseError.NON_BOOTABLE ("Media %s is not bootable.", device_file);
 
+        label = get_udev_property (device, "ID_FS_LABEL_ENC");

How about I just rename get_udev_property to get_decoded_udev_property?

Created attachment 236830 [details] [review]
installer-media: Fetch original volume ID from udev

Renamed get_udev_property to get_decoded_udev_property.

Created attachment 236831 [details] [review]
installer-media: Make use of new udev properties

Renamed get_udev_properties_for_media to get_decoded_udev_properties_for_media.

Review of attachment 236830 [details] [review]:

ack

Review of attachment 236831 [details] [review]:

ack, if you're sure that all the media properties actually are encoded.

Review of attachment 236831 [details] [review]:

They are!

Attachment 236698 [details] pushed as b80bb72 - os-database: Rename guess_os_from_install_media
Attachment 236699 [details] pushed as c01b6fc - os-database: Add guess_os_from_install_media
Attachment 236830 [details] pushed as 43cc580 - installer-media: Fetch original volume ID from udev
Attachment 236831 [details] pushed as 32001c4 - installer-media: Make use of new udev properties

Created attachment 237436 [details] [review]
installer-media: Make use of new udev properties

This should have been part of 32001c4eb950a4b5301d53762a7aff7b139e153f.

Review of attachment 237436 [details] [review]:

::: src/media-manager.vala
@@ +78,3 @@
         var enumerator = new GUdev.Enumerator (client);
+        // We don't want to deal with partitions to avoid duplicate medias
+        enumerator.add_match_property ("DEVTYPE", "disk");

This looks like an unrelated cleanup.

@@ +82,3 @@
         foreach (var device in enumerator.execute ()) {
+            if (device.get_property ("ID_FS_BOOT_SYSTEM_ID") == null &&
+                !device.get_property_as_boolean ("OSINFO_BOOTABLE"))

And this looks like the missing bit for the new udev stuff.

Review of attachment 237436 [details] [review]:

::: src/media-manager.vala
@@ +78,3 @@
         var enumerator = new GUdev.Enumerator (client);
+        // We don't want to deal with partitions to avoid duplicate medias
+        enumerator.add_match_property ("DEVTYPE", "disk");

No, I had to switch the filter condition with the condition below as I don't see any way to have an OR match. i-e i can ask gudev to give me all devices that match condition A and B but not all devices that match condition A *or* B.

So I get all devices matching a condition that should be true for both older and newer udev/libosinfo and below I manually filter for bootable media.

Review of attachment 237436 [details] [review]:

::: src/media-manager.vala
@@ +78,3 @@
         var enumerator = new GUdev.Enumerator (client);
+        // We don't want to deal with partitions to avoid duplicate medias
+        enumerator.add_match_property ("DEVTYPE", "disk");

Ah, didn't realize the add_match_property() was mandatory.

Attachment 237436 [details] pushed as 37a4ac1 - installer-media: Make use of new udev properties