curtin-dev team mailing list archive

[Ryan Harper <rharper@xxxxxxxxxxx>]

Added some inline comments/suggestions.  It would be good to add a unittest that exercises the new path where it invokes udevadm_info() to check the DM_UUID and other changes this is introducing.

Diff comments:

> diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
> index 0cf0866..6ef521f 100644
> --- a/curtin/block/__init__.py
> +++ b/curtin/block/__init__.py
> @@ -158,9 +157,19 @@ def sys_block_path(devname, add=None, strict=True):
>      devname = os.path.normpath(devname)
>      if devname.startswith('/dev/') and not os.path.exists(devname):
>          LOG.warning('block.sys_block_path: devname %s does not exist', devname)
> -    (parent, partnum) = get_blockdev_for_partition(devname, strict=strict)
> -    if partnum:
> -        toks.append(path_to_kname(parent))
> +    if '/' not in devname:
> +        devname = '/dev/' + devname
> +    if os.path.exists(devname):
> +        info = udevadm_info(devname)
> +        uuid = info.get('DM_UUID', '')
> +        if uuid.startswith('part'):
> +            uuid = uuid.split('-', 1)[1]

Lines 23 through 26 looks like something block/multipath.py should handle:

get_mpath_partition_uuid()

> +    else:
> +        uuid = ''
> +    if not uuid.startswith('mpath'):
> +        (parent, partnum) = get_blockdev_for_partition(devname, strict=strict)
> +        if partnum:
> +            toks.append(path_to_kname(parent))

Could this whole hunk be refactored to something like:

if not multipath.is_mpath_partition(devpath):
    (parent, partnum) = get_blockdev_for_partition(devname, strict=strict)
    if partnum:
        toks.append(path_to_kname(parent))

And fix multipath.is_mpath_partition to handle /dev/mapper links via os.path.realpath()

>  
>      toks.append(path_to_kname(devname))
>  
> @@ -421,6 +430,14 @@ def get_blockdev_for_partition(devpath, strict=True):
>      if strict and not os.path.exists(syspath):
>          raise OSError("%s had no syspath (%s)" % (devpath, syspath))
>  
> +    dm_name_path = os.path.join(syspath, 'dm/name')
> +    if os.path.exists(dm_name_path):
> +        dm_name = util.load_file(dm_name_path).rstrip()
> +        if '-part' in dm_name:
> +            parent_name, ptnum = dm_name.rsplit('-part', 1)
> +            parent_path = os.path.realpath('/dev/mapper/' + parent_name)
> +            return (parent_path, ptnum)
> +

Could this go into block/multipath.py as well? and a call to this can be guarded with is_mpath_partition(sysfs_to_devname(syspath))

>      ptpath = os.path.join(syspath, "partition")
>      if not os.path.exists(ptpath):
>          return (rpath, None)


-- 
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/396046
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:fix-multipath-partition-verification into curtin:master.