cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

Scott Moser has proposed merging ~smoser/cloud-init:bug/ds-identify-fix-parse-blkid-export into cloud-init:master.

Commit message:
ds-identify: Fix searching for iso9660 OVF cdroms.

This fixes a bug in parsing of 'blkid -o export' output.  The result
of the bug meant that DI_ISO9660_DEVS did not get set correctly and
is_cdrom_ovf would not identify devices in most cases.

The tests are improved to demonstrate both multiple iso devices
and also a cdrom that doesn't sort "last" in blkid output.

The code change is to use DEVNAME as the record separator when
parsing blkid -o export rather than relying on being able to read
the empty line.

Requested reviews:
  cloud-init commiters (cloud-init-dev)

For more details, see:
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/338470

see commit message
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/ds-identify-fix-parse-blkid-export into cloud-init:master.

diff --git a/tests/unittests/test_ds_identify.py b/tests/unittests/test_ds_identify.py
index 03942de..2125834 100644
--- a/tests/unittests/test_ds_identify.py
+++ b/tests/unittests/test_ds_identify.py
@@ -350,8 +350,10 @@ class TestDsIdentify(CiTestCase):
                             "OVFENV", "ovfenv"]
         for valid_ovf_label in valid_ovf_labels:
             ovf_cdrom_by_label['mocks'][0]['out'] = blkid_out([
+                {'DEVNAME': 'sda1', 'TYPE': 'ext4', 'LABEL': 'rootfs'},
                 {'DEVNAME': 'sr0', 'TYPE': 'iso9660',
-                 'LABEL': valid_ovf_label}])
+                 'LABEL': valid_ovf_label},
+                {'DEVNAME': 'vda1', 'TYPE': 'ntfs', 'LABEL': 'data'}])
             self._check_via_dict(
                 ovf_cdrom_by_label, rc=RC_FOUND, dslist=['OVF', DS_NONE])
 
@@ -513,8 +515,9 @@ VALID_CFG = {
         'mocks': [
             {'name': 'blkid', 'ret': 0,
              'out': blkid_out(
-                 [{'DEVNAME': 'vda1', 'TYPE': 'vfat', 'PARTUUID': uuid4()},
-                  {'DEVNAME': 'sr0', 'TYPE': 'iso9660', 'LABEL': ''}])
+                 [{'DEVNAME': 'sr0', 'TYPE': 'iso9660', 'LABEL': ''},
+                  {'DEVNAME': 'sr1', 'TYPE': 'iso9660', 'LABEL': 'ignoreme'},
+                  {'DEVNAME': 'vda1', 'TYPE': 'vfat', 'PARTUUID': uuid4()}]),
              },
             MOCK_VIRT_IS_VMWARE,
         ],
diff --git a/tools/ds-identify b/tools/ds-identify
index 5f76243..167a85c 100755
--- a/tools/ds-identify
+++ b/tools/ds-identify
@@ -186,7 +186,8 @@ block_dev_with_label() {
 read_fs_info() {
     cached "${DI_BLKID_OUTPUT}" && return 0
     # do not rely on links in /dev/disk which might not be present yet.
-    # note that older blkid versions do not report DEVNAME in 'export' output.
+    # Note that blkid < 2.22 (centos6, trusty) do not output DEVNAME.
+    # that means that DI_ISO9660_DEVS will not be set.
     if is_container; then
         # blkid will in a container, or at least currently in lxd
         # not provide useful information.
@@ -203,21 +204,26 @@ read_fs_info() {
         DI_ISO9660_DEVS="$UNAVAILABLE:error"
         return $ret
     }
-    IFS="$CR"
-    set -- $out
-    IFS="$oifs"
-    for line in "$@" ""; do
+    # 'set --' will collapse multiple consective entries in IFS for
+    # whitespace characters (\n, tab, " ") so we cannot rely on getting
+    # empty lines in "$@" below.
+    IFS="$CR"; set -- $out; IFS="$oifs"
+
+    for line in "$@"; do
         case "${line}" in
-            DEVNAME=*) dev=${line#DEVNAME=};;
+            DEVNAME=*)
+                [ -n "$dev" -a "$ftype" = "iso9660" ] &&
+                    isodevs="${isodevs} ${dev}=$label"
+                ftype=""; dev=""; label="";
+                dev=${line#DEVNAME=};;
             LABEL=*) label="${line#LABEL=}";
                      labels="${labels}${line#LABEL=}${delim}";;
             TYPE=*) ftype=${line#TYPE=};;
-            "") if [ "$ftype" = "iso9660" ]; then
-                    isodevs="${isodevs} ${dev}=$label"
-                fi
-                ftype=""; devname=""; label="";
         esac
     done
+    [ -n "$dev" -a "$ftype" = "iso9660" ] &&
+        isodevs="${isodevs} ${dev}=$label"
+
     DI_FS_LABELS="${labels%${delim}}"
     DI_ISO9660_DEVS="${isodevs# }"
 }
@@ -696,15 +702,12 @@ dscheck_OVF() {
     # Azure provides ovf. Skip false positive by dis-allowing.
     is_azure_chassis && return $DS_NOT_FOUND
 
-    local isodevs="${DI_ISO9660_DEVS}"
-    case "$isodevs" in
-        ""|$UNAVAILABLE:*) return ${DS_NOT_FOUND};;
-    esac
-
     # DI_ISO9660_DEVS is <device>=label, like /dev/sr0=OVF-TRANSPORT
-    for tok in $isodevs; do
-        is_cdrom_ovf "${tok%%=*}" "${tok#*=}" && return $DS_FOUND
-    done
+    if [ "${DI_ISO9660_DEVS#${UNAVAILABLE}:}" = "${DI_ISO9660_DEVS}" ]; then
+        for tok in ${DI_ISO9660_DEVS}; do
+            is_cdrom_ovf "${tok%%=*}" "${tok#*=}" && return $DS_FOUND
+        done
+    fi
 
     if ovf_vmware_guest_customization; then
         return ${DS_FOUND}