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

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(a)linux.vnet.ibm.com&gt; >>> --- >>> 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']: >