[Kimchi-devel] [PATCH v2] logical pool fixes: only list leaf devices, and read file instead of run "cat"
Aline Manera
alinefm at linux.vnet.ibm.com
Tue Dec 24 10:41:58 UTC 2013
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.
>>> +
>>> 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']:
>
More information about the Kimchi-devel
mailing list