From: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com>

The current logical pool implementation has some problems. It lists ext4
patitions but in fact partitions with FS should not be listed, because
a dual boot system setup can make use of those unmounted partitions. It
also fails to run lsblk for some devices.

According to the discussion on github issue page[1]. We decide to list
only unused partitions and disks, which means, PV and LV will not be
listed, mounted partitions and partition with FS will not be listed.

When lsblk fails to run on a purticular device, it swallows the error
and return an empty dict.

Some kinds of free block device still can not be listed by the current
quick fix, for this is only a *quick* fix. The following cases are not
covered, they should be improved in future patches.
1. PV that doesn't belong to any VG.
  blkid and lsblk do not provide enough information to detect if a PV
  belongs to an VG.

To get the device path, it uses sysfs uevent file to get the device name and
then assume the device path is /dev/<devname>

This patch also drops the defensive coding style of the current
implementation. Defensive coding is not Pythonic. It makes the code
harder to debug, delay the discovery of code error and makes the main
logic unclear. This patch also extract a function to run and parse
lsblk to avoid repeating similar code.

For a long term solution, backend should just list all block devices,
along with information describing if they are in use. The front end
provides a checkbox "filter in use devices" and let users decide what
they want.

[1] https://github.com/kimchi-project/kimchi/issues/276

Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com>
Signed-off-by: Aline Manera <alinefm@br.ibm.com>

fix
---
 src/kimchi/disks.py |  170 ++++++++++++++++++++++++++-------------------------
 1 file changed, 87 insertions(+), 83 deletions(-)

diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py
index 874f669..0ae715c 100644
--- a/src/kimchi/disks.py
+++ b/src/kimchi/disks.py
@@ -23,102 +23,106 @@
 import re
 import subprocess

+from kimchi.utils import kimchi_log
 from kimchi.exception import OperationFailed


-def get_partitions_paths():
-    try:
-        """ Returns all available partitions path of the host """
-        blkid = subprocess.Popen(
-            ["blkid", "-o", "device"],
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        dev_paths = blkid.communicate()[0].rstrip("\n").split("\n")
-    except:
-        raise OperationFailed("Unable to retrieve partitions' full path.")
-    return dev_paths
-
-
-def get_partition_path(name, dev_paths):
+def _get_partition_path(name):
     """ Returns device path given a partition name """
     dev_path = None
-    regexp = re.compile(r"^.*"+name+"$")
-    for path in dev_paths:
-        if regexp.search(path) is not None:
-            dev_path = path
+    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
-    if dev_path:
-        return dev_path

+    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)

-def get_partitions_names():
-    try:
-        """ Returns all the names of available partitions  """
-        lsblk = subprocess.Popen(
-            ["lsblk", "-Pbo", "NAME,TYPE"],
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        output = lsblk.communicate()[0]
-        lines_output = output.rstrip("\n").split("\n")
-        # Will be used later to double check the partition
-        dev_paths = get_partitions_paths()
-        names = []
-        keys = ["NAME", "TYPE"]
-        # output is on format key="value", where key = NAME, TYPE
-        for line in lines_output:
-            d = {}
-            for key in keys:
-                expression = r"%s=\".*?\"" % key
-                match = re.search(expression, line)
-                field = match.group()
-                k, v = field.split('=', 1)
-                d[k.lower()] = v[1:-1]
-            if d['type'] not in ['part', 'lvm']:
-                continue
-            # split()[0] to avoid the second part of the name,
-            # after the whiteline
-            name = d['name'].split()[0]
-            # There are special cases where lsblk reports
-            # a partition that does not exist in blkid and fdisk (Extended
-            # partitions), which can't be used for pool creation. We
-            # need to filter these cases as well.
-            if not get_partition_path(name, dev_paths):
-                continue
-            names.append(name)
-        return names
-    except:
-        raise OperationFailed("Unable to retrieve partitions' full path.")
+    data = dict(re.findall(r'(\S+)=(".*?"|\S+)', out.replace("\n", " ")))

+    return "/dev/%s" % data["DEVNAME"]

-def get_partition_details(name):
-    try:
-        # Find device path
-        dev_path = get_partition_path(name, get_partitions_paths())
-        # Couldn't find dev_path.
-        if not dev_path:
-            return
-        # Executing lsblk to get partition/disk info
-        lsblk = subprocess.Popen(
-            ["lsblk", "-Pbo", "TYPE,FSTYPE,SIZE,MOUNTPOINT", dev_path],
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        # single line output
-        output = lsblk.communicate()[0].rstrip("\n")
-        # output is on format key="value", where key = NAME, TYPE, FSTYPE,
-        # SIZE, MOUNTPOINT
-        keys = ["TYPE", "FSTYPE", "SIZE", "MOUNTPOINT"]
+
+def _get_lsblk_devs(keys, devs=[]):
+    lsblk = subprocess.Popen(
+        ["lsblk", "-Pbo"] + [','.join(keys)] + devs,
+        stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    out, err = lsblk.communicate()
+    if lsblk.returncode != 0:
+        raise OperationFailed('Error executing lsblk: %s' % err)
+
+    return _parse_lsblk_output(out, keys)
+
+
+def _parse_lsblk_output(output, keys):
+    # output is on format key="value",
+    # where key can be NAME, TYPE, FSTYPE, SIZE, MOUNTPOINT, etc
+    lines = output.rstrip("\n").split("\n")
+    r = []
+    for line in lines:
         d = {}
         for key in keys:
             expression = r"%s=\".*?\"" % key
-            match = re.search(expression, output)
+            match = re.search(expression, line)
             field = match.group()
             k, v = field.split('=', 1)
             d[k.lower()] = v[1:-1]
-        if d['mountpoint']:
-            # Sometimes the mountpoint comes with [SWAP] or other
-            # info which is not an actual mount point. Filtering it
-            regexp = re.compile(r"\[.*\]")
-            if regexp.search(d['mountpoint']) is not None:
-                d['mountpoint'] = ''
-        d['path'] = dev_path
-        d['name'] = name
-        return d
-    except:
-        raise OperationFailed("Unable to retrieve partitions' data.")
+        r.append(d)
+    return r
+
+
+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
+    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])

+            continue
+
+        names.append(name)
+
+    return list(set(names) - set(ignore_names))
+
+
+def get_partition_details(name):
+    dev_path = _get_partition_path(name)
+
+    keys = ["TYPE", "FSTYPE", "SIZE", "MOUNTPOINT"]
+    try:
+        dev = _get_lsblk_devs(keys, [dev_path])[0]
+    except OperationFailed as e:
+        kimchi_log.error(
+            "Error getting partition info for %s: %s", (name, e))

should be:
+        kimchi_log.error(
+            "Error getting partition info for %s: %s", name, e)

+        return {}
+
+    if dev['mountpoint']:
+        # Sometimes the mountpoint comes with [SWAP] or other
+        # info which is not an actual mount point. Filtering it
+        regexp = re.compile(r"\[.*\]")
+        if regexp.search(dev['mountpoint']) is not None:
+            dev['mountpoint'] = ''
+    dev['path'] = dev_path
+    dev['name'] = name
+    return dev

Sheldon Feng(冯少合)
IBM Linux Technology Center