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

[Zhou Zheng Sheng]

on 2013/12/24 13:15, Sheldon wrote:
> Reviewed-by: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
> Test-by: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
> 
Thanks. I also tested it in my Fedora 19 guest.
> 
> On 12/23/2013 10:44 PM, 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.
>>
>> 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..34b9e01 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
>> +    else:
>> +        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:
> 
> 


-- 
Thanks and best regards!

Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397