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

Anne Mulhern amulhern at redhat.com
Wed Sep 24 12:47:23 UTC 2014





----- Original Message -----
> From: "Will Woods" <wwoods at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Tuesday, September 23, 2014 6:14:00 PM
> Subject: [PATCH blivet/master 1/4] devicelibs.lvm: refactor	_getConfigArgs()/lvm()
> 
> 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
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

This looks good to me, except that there are place where you now
silently catch an LVMError (thinlvpoolname, lvorigin, lvs)
and return an empty value in that case, where previously an exception
would be raised if the call failed due to an OSError.

Your code doesn't change the use of an empty value to mean lvm failed to get anything,
it just makes that empty value (as opposed to a raised exception) more likely. 

You should definitely document this new behaviour explicitly in
the method headers and the commit message.

Or else, maybe the decision needs to be re-evaluated and the exception should be propagated,
instead.

It is hard for me to tell which is better.
These methods are all purely informational, so maybe they
should never raise an exception, on the other hand, pvscan is also purely informational
and it does. pvinfo does not raise an LVMError, but vginfo does.

I think uses of lvorigin are doing the right thing wrt. to an empty return value,
uses of thinlvpoolname are probably in a bad case, and uses of lvs
may be broken.

- mulhern





More information about the anaconda-patches mailing list