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

[Zhou Zheng Sheng]

on 2013/12/23 19:25, Aline Manera wrote:
> 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 ?
> 
Thanks Aline. "try" block can have an "else" block. The "else" block is
executed if the "try" block does not raise an exception.

To get more information, you can see the following and search "else" in
page.
http://docs.python.org/2/tutorial/errors.html

By the way, a "for" block can also followed by an "else" block, for example.

for aKey in aDict:
  if blah(aKey):
    result = aDict[aKey]
    break
else:
  result = aDefaultValue

print result

The "else" block is executed when the "for" loop ends normally. If it
breaks the "for" loop, "else" block is skipped.

I like these Pythonic constructs very much. They make the code more
readable.
>> +
>>   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']:
> 


-- 
Thanks and best regards!

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