[Kimchi-devel] [PATCH v2] logical pool fixes: only list leaf devices, and read file instead of run "cat"
Zhou Zheng Sheng
zhshzhou at linux.vnet.ibm.com
Thu Dec 26 02:12:55 UTC 2013
on 2013/12/24 18:41, Aline Manera wrote:
> On 12/23/2013 12:58 PM, Zhou Zheng Sheng wrote:
>> 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.
>
> Yes, but the 'else' statement isn't needed as what is in this block is
> the normal flow.
>
Thanks. Let me update it in v3. Any other comments on this patch series?
>>>> +
>>>> 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
More information about the Kimchi-devel
mailing list