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. 2. Devices not listed by "blkid -o device" Some devices are not listed by "blkid -o device", but actually they can be used. For example, a disk without patition table. Disk with partitions are not listed by blkid but the partitions are listed, this is OK. However, a disk with partition table but with no patitions is not listed in "blkid -o device", while it can be used actually. 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(a)linux.vnet.ibm.com&gt; --- src/kimchi/disks.py | 168 ++++++++++++++++++++++++++++------------------------ 1 file changed, 92 insertions(+), 76 deletions(-) diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py index 874f669..7c7798a 100644 --- a/src/kimchi/disks.py +++ b/src/kimchi/disks.py @@ -23,22 +23,25 @@ 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.") +def _get_partitions_paths(): + """ Returns all available partitions path of the host """ + blkid = subprocess.Popen( + ["blkid", "-o", "device"], + stdout=subprocess.PIPE, stderr=subprocess.PIPE) + out, err = blkid.communicate() + if blkid.returncode != 0: + raise OperationFailed('Error executing blkid: %s' % err) + + dev_paths = out.rstrip("\n").split("\n") + return dev_paths -def get_partition_path(name, dev_paths): +def _get_partition_path(name, dev_paths): """ Returns device path given a partition name """ dev_path = None regexp = re.compile(r"^.*"+name+"$") @@ -46,79 +49,92 @@ def get_partition_path(name, dev_paths): if regexp.search(path) is not None: dev_path = path break - if dev_path: - return dev_path + return dev_path -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.") +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 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 _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(): + # Will be used later to double check the partition + dev_paths = _get_partitions_paths() + 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] + # 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. + path = _get_partition_path(name, dev_paths) + + # Only list unmounted and unformated partition or disk. + if not all([dev['type'] in ['part', 'disk'], + dev['fstype'] == "", + dev['mountpoint'] == "", + path is not None]): + continue + + names.append(name) + return names + + +def get_partition_details(name): + # Find device path + try: + dev_paths = _get_partitions_paths() + except OperationFailed as e: + kimchi_log.error( + "Error getting partition path for %s: %s", (name, e)) + return {} + + dev_path = _get_partition_path(name, dev_paths) + if dev_path is None: + return {} + + 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)) + 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 -- 1.7.11.7 -- project-kimchi mailing list <project-kimchi(a)googlegroups.com&gt; https://groups.google.com/forum/#!forum/project-kimchi --- You received this message because you are subscribed to the Google Groups "project-kimchi" group. To unsubscribe from this group and stop receiving emails from it, send an email to project-kimchi+unsubscribe(a)googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.

Reviewed-by: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; On 12/18/2013 06:20 PM, Zhou Zheng Sheng wrote: Sheldon Feng(???) IBM Linux Technology Center -- project-kimchi mailing list <project-kimchi(a)googlegroups.com&gt; https://groups.google.com/forum/#!forum/project-kimchi --- You received this message because you are subscribed to the Google Groups "project-kimchi" group. To unsubscribe from this group and stop receiving emails from it, send an email to project-kimchi+unsubscribe(a)googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.