[PATCH blivet/master 1/4] devicelibs.lvm: refactor _getConfigArgs()/lvm()

Will Woods wwoods at redhat.com
Tue Sep 23 22:14:00 UTC 2014


Every place in devicelibs.lvm that executes lvm uses _getConfigArgs()[1] to
help build the argument list. So it'd be cleaner and simpler if we made
every call to the lvm binary go through lvm(), and then just called
_getConfigArgs() from there.

This also means we can pass the caller-requested lvm command+arguments to
_getConfigArgs(), so it can examine them and add appropriate config options
for us.

For example: right now the caller has to explicitly request
read_only_locking if it wants read-only locks. The current code does that
for every call to the informational commands (lvs, pvs, vgs, etc.).

This change moves that logic into _getConfigArgs(), which now just
automatically does read-only locking whenever we call one of the info
commands. Much simpler!

[1] except thinsnapshotcreate(), but that was an oversight (which is
therefore fixed by this commit)
---
 blivet/devicelibs/lvm.py | 122 +++++++++++++++++++++++------------------------
 1 file changed, 60 insertions(+), 62 deletions(-)

diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py
index 87d2639..f1e14f3 100644
--- a/blivet/devicelibs/lvm.py
+++ b/blivet/devicelibs/lvm.py
@@ -63,11 +63,15 @@ def has_lvm():
 config_args_data = { "filterRejects": [],    # regular expressions to reject.
                      "filterAccepts": [] }   # regexp to accept
 
-def _getConfigArgs(**kwargs):
+def _getConfigArgs(args):
     """lvm command accepts lvm.conf type arguments preceded by --config. """
-    config_args = []
 
-    read_only_locking = kwargs.get("read_only_locking", False)
+    # These commands are read-only, so we can run them with read-only locking.
+    # (not an exhaustive list, but these are the only ones used here)
+    READONLY_COMMANDS = ('lvs', 'pvs', 'vgs')
+
+    cmd = args[0]
+    config_args = []
 
     filter_string = ""
     rejects = config_args_data["filterRejects"]
@@ -87,7 +91,7 @@ def _getConfigArgs(**kwargs):
     # "preferred_names", "filter", "cache_dir", "write_cache_state",
     # "types", "sysfs_scan", "md_component_detection".  see man lvm.conf.
     config_string = " devices { %s } " % (devices_string) # strings can be added
-    if read_only_locking:
+    if cmd in READONLY_COMMANDS:
         config_string += "global {locking_type=4} "
     if config_string:
         config_args = ["--config", config_string]
@@ -204,18 +208,25 @@ def is_valid_thin_pool_chunk_size(size, discard=False):
     else:
         return (size % LVM_THINP_MIN_CHUNK_SIZE == 0)
 
-def lvm(args):
-    ret = util.run_program(["lvm"] + args)
+def lvm(args, capture=False):
+    """ Runs lvm with specified arguments.
+
+        :param bool capture: if True, return the output of the command.
+        :returns: the output of the command or None
+        :rtype: list of str or NoneType
+        :raises: LVMError if command fails
+    """
+    argv = ["lvm"] + args + _getConfigArgs(args)
+    (ret, out) = util.run_program_and_capture_output(argv)
     if ret:
-        raise LVMError("running lvm " + " ".join(args) + " failed")
+        raise LVMError("running "+ " ".join(argv) + " failed")
+    if capture:
+        return out
 
 def pvcreate(device):
     # we force dataalignment=1024k since we cannot get lvm to tell us what
     # the pe_start will be in advance
-    args = ["pvcreate"] + \
-            _getConfigArgs() + \
-            ["--dataalignment", "1024k"] + \
-            [device]
+    args = ["pvcreate", "--dataalignment", "1024k", device]
 
     try:
         lvm(args)
@@ -223,10 +234,9 @@ def pvcreate(device):
         raise LVMError("pvcreate failed for %s: %s" % (device, msg))
 
 def pvresize(device, size):
-    args = ["pvresize"] + \
-            ["--setphysicalvolumesize", ("%dm" % size.convertTo(spec="mib"))] + \
-            _getConfigArgs() + \
-            [device]
+    args = ["pvresize",
+            "--setphysicalvolumesize", ("%dm" % size.convertTo(spec="mib")),
+            device]
 
     try:
         lvm(args)
@@ -234,9 +244,7 @@ def pvresize(device, size):
         raise LVMError("pvresize failed for %s: %s" % (device, msg))
 
 def pvremove(device):
-    args = ["pvremove", "--force", "--force", "--yes"] + \
-            _getConfigArgs() + \
-            [device]
+    args = ["pvremove", "--force", "--force", "--yes", device]
 
     try:
         lvm(args)
@@ -244,9 +252,7 @@ def pvremove(device):
         raise LVMError("pvremove failed for %s: %s" % (device, msg))
 
 def pvscan(device):
-    args = ["pvscan", "--cache",] + \
-            _getConfigArgs() + \
-            [device]
+    args = ["pvscan", "--cache", device]
 
     try:
         lvm(args)
@@ -259,7 +265,7 @@ def pvmove(source, dest=None):
         :param str source: pv device path to move extents off of
         :keyword str dest: pv device path to move the extents onto
     """
-    args = ["pvmove"] + _getConfigArgs() + [source]
+    args = ["pvmove", source]
     if dest:
         args.append(dest)
 
@@ -304,13 +310,16 @@ def pvinfo(device=None):
             "--unit=k", "--nosuffix", "--nameprefixes",
             "--unquoted", "--noheadings",
             "-opv_name,pv_uuid,pe_start,vg_name,vg_uuid,vg_size,vg_free,"
-            "vg_extent_size,vg_extent_count,vg_free_count,pv_count"] + \
-            _getConfigArgs(read_only_locking=True)
+            "vg_extent_size,vg_extent_count,vg_free_count,pv_count"]
 
     if device:
         args.append(device)
 
-    buf = util.capture_output(["lvm"] + args)
+    try:
+        buf = lvm(args, capture=True)
+    except LVMError:
+        return {}
+
     pvs = {}
     for line in buf.splitlines():
         info = parse_lvm_vars(line)
@@ -326,7 +335,6 @@ def vgcreate(vg_name, pv_list, pe_size):
     argv = ["vgcreate"]
     if pe_size:
         argv.extend(["-s", "%dm" % pe_size.convertTo(spec="mib")])
-    argv.extend(_getConfigArgs())
     argv.append(vg_name)
     argv.extend(pv_list)
 
@@ -336,9 +344,7 @@ def vgcreate(vg_name, pv_list, pe_size):
         raise LVMError("vgcreate failed for %s: %s" % (vg_name, msg))
 
 def vgremove(vg_name):
-    args = ["vgremove", "--force"] + \
-            _getConfigArgs() +\
-            [vg_name]
+    args = ["vgremove", "--force", vg_name]
 
     try:
         lvm(args)
@@ -346,9 +352,7 @@ def vgremove(vg_name):
         raise LVMError("vgremove failed for %s: %s" % (vg_name, msg))
 
 def vgactivate(vg_name):
-    args = ["vgchange", "-a", "y"] + \
-            _getConfigArgs() + \
-            [vg_name]
+    args = ["vgchange", "-a", "y", vg_name]
 
     try:
         lvm(args)
@@ -356,9 +360,7 @@ def vgactivate(vg_name):
         raise LVMError("vgactivate failed for %s: %s" % (vg_name, msg))
 
 def vgdeactivate(vg_name):
-    args = ["vgchange", "-a", "n"] + \
-            _getConfigArgs() + \
-            [vg_name]
+    args = ["vgchange", "-a", "n", vg_name]
 
     try:
         lvm(args)
@@ -380,7 +382,6 @@ def vgreduce(vg_name, pv, missing=False):
             it from the VG. You must do that first by calling :func:`.pvmove`.
     """
     args = ["vgreduce"]
-    args.extend(_getConfigArgs())
     if missing:
         args.extend(["--removemissing", "--force", vg_name])
     else:
@@ -397,7 +398,7 @@ def vgextend(vg_name, pv):
         :param str vg_name: the name of the VG
         :param str pv: device path of PV to add
     """
-    args = ["vgextend"] + _getConfigArgs() + [vg_name, pv]
+    args = ["vgextend", vg_name, pv]
 
     try:
         lvm(args)
@@ -412,10 +413,14 @@ def vginfo(vg_name):
     """
     args = ["vgs", "--noheadings", "--nosuffix", "--nameprefixes", "--unquoted",
             "--units", "m",
-            "-o", "uuid,size,free,extent_size,extent_count,free_count,pv_count"] + \
-            _getConfigArgs(read_only_locking=True) + [vg_name]
+            "-o", "uuid,size,free,extent_size,extent_count,free_count,pv_count",
+            vg_name]
+
+    try:
+        buf = lvm(args, capture=True)
+    except LVMError:
+        buf = ''
 
-    buf = util.capture_output(["lvm"] + args)
     info = parse_lvm_vars(buf)
     if len(info.keys()) != 7:
         raise LVMError(_("vginfo failed for %s") % vg_name)
@@ -434,12 +439,15 @@ def lvs(vg_name=None):
     args = ["lvs",
             "-a", "--unit", "k", "--nosuffix", "--nameprefixes",
             "--unquoted", "--noheadings",
-            "-ovg_name,lv_name,lv_uuid,lv_size,lv_attr,segtype"] + \
-            _getConfigArgs(read_only_locking=True)
+            "-ovg_name,lv_name,lv_uuid,lv_size,lv_attr,segtype"]
     if vg_name:
         args.append(vg_name)
 
-    buf = util.capture_output(["lvm"] + args)
+    try:
+        buf = lvm(args, capture=True)
+    except LVMError:
+        return {}
+
     logvols = {}
     for line in buf.splitlines():
         info = parse_lvm_vars(line)
@@ -454,14 +462,12 @@ def lvs(vg_name=None):
 
 def lvorigin(vg_name, lv_name):
     args = ["lvs", "--noheadings", "-o", "origin"] + \
-            _getConfigArgs(read_only_locking=True) + \
             ["%s/%s" % (vg_name, lv_name)]
 
-    buf = util.capture_output(["lvm"] + args)
-
     try:
+        buf = lvm(args, capture=True)
         origin = buf.splitlines()[0].strip()
-    except IndexError:
+    except (IndexError, LVMError):
         origin = ''
 
     return origin
@@ -473,7 +479,6 @@ def lvcreate(vg_name, lv_name, size, pvs=None):
             ["-L", "%dm" % size.convertTo(spec="mib")] + \
             ["-n", lv_name] + \
             ["-y"] + \
-            _getConfigArgs() + \
             [vg_name] + pvs
 
     try:
@@ -486,7 +491,7 @@ def lvremove(vg_name, lv_name, force=False):
     if force:
         args.extend(["--force", "--yes"])
 
-    args += _getConfigArgs() + ["%s/%s" % (vg_name, lv_name)]
+    args += ["%s/%s" % (vg_name, lv_name)]
 
     try:
         lvm(args)
@@ -496,7 +501,6 @@ def lvremove(vg_name, lv_name, force=False):
 def lvresize(vg_name, lv_name, size):
     args = ["lvresize"] + \
             ["--force", "-L", "%dm" % size.convertTo(spec="mib")] + \
-            _getConfigArgs() + \
             ["%s/%s" % (vg_name, lv_name)]
 
     try:
@@ -510,7 +514,7 @@ def lvactivate(vg_name, lv_name, ignore_skip=False):
     if ignore_skip:
         args.append("-K")
 
-    args += _getConfigArgs() + ["%s/%s" % (vg_name, lv_name)]
+    args += ["%s/%s" % (vg_name, lv_name)]
 
     try:
         lvm(args)
@@ -519,7 +523,6 @@ def lvactivate(vg_name, lv_name, ignore_skip=False):
 
 def lvdeactivate(vg_name, lv_name):
     args = ["lvchange", "-a", "n"] + \
-            _getConfigArgs() + \
             ["%s/%s" % (vg_name, lv_name)]
 
     try:
@@ -536,8 +539,7 @@ def lvsnapshotmerge(vg_name, lv_name):
             how merge is handled by lvm.
 
     """
-    args = ["lvconvert", "--merge", "%s/%s" % (vg_name, lv_name)]
-    args += _getConfigArgs() + [lv_name]
+    args = ["lvconvert", "--merge", "%s/%s" % (vg_name, lv_name), lv_name]
 
     try:
         lvm(args)
@@ -552,7 +554,7 @@ def lvsnapshotcreate(vg_name, snap_name, size, origin_name):
         :param str origin_name: the name of the origin logical volume
     """
     args = ["lvcreate", "-s", "-L", "%dm" % size.convertTo(spec="MiB"),
-            "-n", snap_name] + _getConfigArgs() + ["%s/%s" % (vg_name, origin_name)]
+            "-n", snap_name, "%s/%s" % (vg_name, origin_name)]
 
     try:
         lvm(args)
@@ -571,8 +573,6 @@ def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None):
         # default unit is KiB
         args += ["--chunksize", "%d" % chunksize.convertTo(spec="kib")]
 
-    args += _getConfigArgs()
-
     try:
         lvm(args)
     except LVMError as msg:
@@ -581,8 +581,7 @@ def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None):
 def thinlvcreate(vg_name, pool_name, lv_name, size):
     args = ["lvcreate", "--thinpool", "%s/%s" % (vg_name, pool_name),
             "--virtualsize", "%dm" % size.convertTo(spec="MiB"),
-            "-n", lv_name] + \
-            _getConfigArgs()
+            "-n", lv_name]
 
     try:
         lvm(args)
@@ -606,14 +605,13 @@ def thinsnapshotcreate(vg_name, snap_name, origin_name, pool_name=None):
 
 def thinlvpoolname(vg_name, lv_name):
     args = ["lvs", "--noheadings", "-o", "pool_lv"] + \
-            _getConfigArgs(read_only_locking=True) + \
             ["%s/%s" % (vg_name, lv_name)]
 
-    buf = util.capture_output(["lvm"] + args)
 
     try:
+        buf = lvm(args, capture=True)
         pool = buf.splitlines()[0].strip()
-    except IndexError:
+    except (IndexError, LVMError):
         pool = ''
 
     return pool
-- 
1.9.3



More information about the anaconda-patches mailing list