[Kimchi-devel] [PATCH v2] logical pool fixes: only list leaf devices, and read file instead of run "cat"

[Aline Manera]

On 12/20/2013 05:36 AM, Zhou Zheng Sheng wrote:
> Some devices are parent of other devices. We should just list the leaf
> devices but not parent devices. For example, if a disk contains
> partitions, we should only list the partitions. If a disk is held by
> multipath device, we should not list the disk.
>
> Currently we strip the last charactter from "vda1" to get "vda" and
> ignore the parent "vda" device. This may fails on disk contains lots of
> logical partitions, for example "vda10"'s parent is not "vda1". Another
> problem is this method can not find the parent-child relation ship
> between a multipath device and its slaves.
>
> The most accurate information is on sysfs, and lsblk lists all children
> device of the requested device based on sysfs. So when "lsblk -P
> devices" prints only one line, it's the device itself, when it prints
> more lines, they are the children devices. This patch use this method to
> detect if a device a leaf one.
>
> This patch also avoids start new "cat" process to read uevent file, it
> opens the uevent file and read it directly.
>
> v2:
>    Assign string literal to constants thus avoid breaking the line.
>
> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
> ---
>   src/kimchi/disks.py | 48 ++++++++++++++++++++++++++++++------------------
>   1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py
> index 991bb4a..7bba8b6 100644
> --- a/src/kimchi/disks.py
> +++ b/src/kimchi/disks.py
> @@ -29,7 +29,6 @@ from kimchi.utils import kimchi_log
>
>   def _get_partition_path(name):
>       """ Returns device path given a partition name """
> -    dev_path = None
>       maj_min = None
>
>       keys = ["NAME", "MAJ:MIN"]
> @@ -39,15 +38,15 @@ def _get_partition_path(name):
>           if dev['name'] == name:
>               maj_min = dev['maj:min']
>               break
> +    else:
> +        msg = "Failed to find major and minor number for %s" % name
> +        raise OperationFailed(msg)
>
> -    uevent_cmd = "cat /sys/dev/block/%s/uevent" % maj_min
> -    uevent = subprocess.Popen(uevent_cmd, stdout=subprocess.PIPE,
> -                              stderr=subprocess.PIPE, shell=True)
> -    out, err = uevent.communicate()
> -    if uevent.returncode != 0:
> -        raise OperationFailed("Error getting partition path for %s", name)
> +    uevent = "/sys/dev/block/%s/uevent" % maj_min
> +    with open(uevent) as ueventf:
> +        content = ueventf.read()
>
> -    data = dict(re.findall(r'(\S+)=(".*?"|\S+)', out.replace("\n", " ")))
> +    data = dict(re.findall(r'(\S+)=(".*?"|\S+)', content.replace("\n", " ")))
>
>       return "/dev/%s" % data["DEVNAME"]
>
> @@ -63,6 +62,22 @@ def _get_lsblk_devs(keys, devs=[]):
>       return _parse_lsblk_output(out, keys)
>
>
> +def _is_dev_leaf(name):
> +    try:
> +        # By default, lsblk prints a device information followed by children
> +        # device information
> +        childrenCount = len(
> +            _get_lsblk_devs(["NAME"], [_get_partition_path(name)])) - 1
> +    except OperationFailed as e:
> +        # lsblk is known to fail on multipath devices
> +        # Assume these devices contain children
> +        kimchi_log.error(
> +            "Error getting device info for %s: %s", name, e)
> +        return False
> +    else:
> +        return childrenCount == 0
> +

else ?

> +
>   def _parse_lsblk_output(output, keys):
>       # output is on format key="value",
>       # where key can be NAME, TYPE, FSTYPE, SIZE, MOUNTPOINT, etc
> @@ -82,7 +97,6 @@ def _parse_lsblk_output(output, keys):
>
>   def get_partitions_names():
>       names = []
> -    ignore_names = []
>       keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT"]
>       # output is on format key="value",
>       # where key can be NAME, TYPE, FSTYPE, MOUNTPOINT
> @@ -90,20 +104,18 @@ def get_partitions_names():
>           # split()[0] to avoid the second part of the name, after the
>           # whiteline
>           name = dev['name'].split()[0]
> -        # Only list unmounted and unformated partition or disk.
> +        # Only list unmounted and unformated and leaf and (partition or disk)
> +        # leaf means a partition, a disk has no partition, or a disk not held
> +        # by any multipath device.
>           if not all([dev['type'] in ['part', 'disk'],
>                       dev['fstype'] == "",
> -                    dev['mountpoint'] == ""]):
> -
> -            # the whole disk must be ignored in it has at least one
> -            # mounted/formatted partition
> -            if dev['type'] == 'part':
> -                ignore_names.append(name[:-1])
> +                    dev['mountpoint'] == "",
> +                    _is_dev_leaf(name)]):
>               continue
>
>           names.append(name)
>
> -    return list(set(names) - set(ignore_names))
> +    return names
>
>
>   def get_partition_details(name):
> @@ -114,7 +126,7 @@ def get_partition_details(name):
>           dev = _get_lsblk_devs(keys, [dev_path])[0]
>       except OperationFailed as e:
>           kimchi_log.error(
> -            "Error getting partition info for %s: %s", (name, e))
> +            "Error getting partition info for %s: %s", name, e)
>           return {}
>
>       if dev['mountpoint']: