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

[Aline Manera]

Reviewed-by: Aline Manera <alinefm at linux.vnet.ibm.com>

On 12/27/2013 08:27 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.
>
> v3:
>    Save N lsblk invocations when list all devices by collecting major:min
>    and device node path in one run. N is number of block devices.
>    Use short-circuiting operator "and" instead of all to save extra lsblk
>    invocation.
>
> v4:
>    Remove a redundant "else" as suggested by Aline.
>
> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
> ---
>   src/kimchi/disks.py | 83 ++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 50 insertions(+), 33 deletions(-)
>
> diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py
> index a054961..a7e5616 100644
> --- a/src/kimchi/disks.py
> +++ b/src/kimchi/disks.py
> @@ -27,27 +27,13 @@ from kimchi.exception import OperationFailed
>   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"]
> -    dev_list = _get_lsblk_devs(keys)
> -
> -    for dev in dev_list:
> -        if dev['name'] == name:
> -            maj_min = dev['maj:min']
> -            break
> +def _get_dev_node_path(maj_min):
> +    """ Returns device node path given the device number 'major:min' """
> +    uevent = "/sys/dev/block/%s/uevent" % maj_min
> +    with open(uevent) as ueventf:
> +        content = ueventf.read()
>
> -    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)
> -
> -    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 +49,39 @@ def _get_lsblk_devs(keys, devs=[]):
>       return _parse_lsblk_output(out, keys)
>
>
> +def _get_dev_major_min(name):
> +    maj_min = None
> +
> +    keys = ["NAME", "MAJ:MIN"]
> +    dev_list = _get_lsblk_devs(keys)
> +
> +    for dev in dev_list:
> +        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)
> +
> +    return maj_min
> +
> +
> +def _is_dev_leaf(devNodePath):
> +    try:
> +        # By default, lsblk prints a device information followed by children
> +        # device information
> +        childrenCount = len(
> +            _get_lsblk_devs(["NAME"], [devNodePath])) - 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", devNodePath, e)
> +        return False
> +
> +    return childrenCount == 0
> +
> +
>   def _parse_lsblk_output(output, keys):
>       # output is on format key="value",
>       # where key can be NAME, TYPE, FSTYPE, SIZE, MOUNTPOINT, etc
> @@ -82,32 +101,30 @@ def _parse_lsblk_output(output, keys):
>
>   def get_partitions_names():
>       names = []
> -    ignore_names = []
> -    keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT"]
> +    keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT", "MAJ:MIN"]
>       # output is on format key="value",
>       # where key can be NAME, TYPE, FSTYPE, MOUNTPOINT
>       for dev in _get_lsblk_devs(keys):
>           # 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.
> -        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])
> +        devNodePath = _get_dev_node_path(dev['maj:min'])
> +        # 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 (dev['type'] in ['part', 'disk'] and
> +                dev['fstype'] == "" and
> +                dev['mountpoint'] == "" and
> +                _is_dev_leaf(devNodePath)):
>               continue
>
>           names.append(name)
>
> -    return list(set(names) - set(ignore_names))
> +    return names
>
>
>   def get_partition_details(name):
> -    dev_path = _get_partition_path(name)
> +    dev_path = _get_dev_node_path(_get_dev_major_min(name))
>
>       keys = ["TYPE", "FSTYPE", "SIZE", "MOUNTPOINT"]
>       try: