Re: [Libguestfs] [PATCH v2 3/5] daemon: move Lvm.lv_canonical to new Lvm_utils module

Richard W.M. Jones
2018-04-10T10:48:28Z

On Tue, Apr 10, 2018 at 12:42:52PM +0200, Pino Toscano
wrote:
> This way the Lvm module contains only the OCaml implementations of LVM
> daemon APIs.
>
> This is simple refactoring, with no functional changes.

Looks like simple code motion as you say, so ACK.

Rich.

>  daemon/Makefile.am              |  2 ++
>  daemon/findfs.ml                |  2 +-
>  daemon/inspect_fs_unix_fstab.ml |  2 +-
>  daemon/lvm.ml                   | 27 -----------------------
>  daemon/lvm.mli                  | 10 ---------
>  daemon/lvm_utils.ml             | 48
+++++++++++++++++++++++++++++++++++++++++
>  daemon/lvm_utils.mli            | 27 +++++++++++++++++++++++
>  7 files changed, 79 insertions(+), 39 deletions(-)
>  create mode 100644 daemon/lvm_utils.ml
>  create mode 100644 daemon/lvm_utils.mli
>
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index 9dbd375f5..9cd34ff75 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -268,6 +268,7 @@ SOURCES_MLI = \
>  	link.mli \
>  	listfs.mli \
>  	lvm.mli \
> +	lvm_utils.mli \
>  	md.mli \
>  	mount.mli \
>  	mountable.mli \
> @@ -296,6 +297,7 @@ SOURCES_ML = \
>  	ldm.ml \
>  	link.ml \
>  	lvm.ml \
> +	lvm_utils.ml \
>  	findfs.ml \
>  	md.ml \
>  	mount.ml \
> diff --git a/daemon/findfs.ml b/daemon/findfs.ml
> index f613015f0..c24a194e3 100644
> --- a/daemon/findfs.ml
> +++ b/daemon/findfs.ml
> @@ -44,7 +44,7 @@ and findfs tag str >
>    if String.is_prefix out "/dev/mapper/" ||
>       String.is_prefix out "/dev/dm-" then (
> -    match Lvm.lv_canonical out with
> +    match Lvm_utils.lv_canonical out with
>      | None ->
>         (* Ignore the case where 'out' doesn't appear to be an
LV.
>          * The best we can do is return the original as-is.
> diff --git a/daemon/inspect_fs_unix_fstab.ml
b/daemon/inspect_fs_unix_fstab.ml
> index 43cec2323..edb797e3f 100644
> --- a/daemon/inspect_fs_unix_fstab.ml
> +++ b/daemon/inspect_fs_unix_fstab.ml
> @@ -327,7 +327,7 @@ and resolve_fstab_device spec md_map os_type >
* we have implemented lvm_canonical_lv_name in the daemon.
>       *)
>      try
> -      match Lvm.lv_canonical spec with
> +      match Lvm_utils.lv_canonical spec with
>        | None -> Mountable.of_device spec
>        | Some device -> Mountable.of_device device
>      with
> diff --git a/daemon/lvm.ml b/daemon/lvm.ml
> index ed4ed7462..ef45ed4bc 100644
> --- a/daemon/lvm.ml
> +++ b/daemon/lvm.ml
> @@ -97,30 +97,3 @@ and filter_convert_old_lvs_output out >    ) lines in
>
>    List.sort compare lines
> -
> -(* Convert a non-canonical LV path like /dev/mapper/vg-lv or /dev/dm-0
> - * to a canonical one.
> - *
> - * This is harder than it should be.  A LV device like /dev/VG/LV is
> - * really a symlink to a device-mapper device like /dev/dm-0.  However
> - * at the device-mapper (kernel) level, nothing is really known about
> - * LVM (a userspace concept).  Therefore we use a convoluted method to
> - * determine this, by listing out known LVs and checking whether the
> - * rdev (major/minor) of the device we are passed matches any of them.
> - *
> - * Note use of 'stat' instead of 'lstat' so that symlinks
are fully
> - * resolved.
> - *)
> -let lv_canonical device > -  let stat1 = stat device in
> -  let lvs = lvs () in
> -  try
> -    Some (
> -      List.find (
> -        fun lv ->
> -          let stat2 = stat lv in
> -          stat1.st_rdev = stat2.st_rdev
> -      ) lvs
> -    )
> -  with
> -  | Not_found -> None
> diff --git a/daemon/lvm.mli b/daemon/lvm.mli
> index e9a6faeca..592168433 100644
> --- a/daemon/lvm.mli
> +++ b/daemon/lvm.mli
> @@ -17,13 +17,3 @@
>   *)
>
>  val lvs : unit -> string list
> -
> -val lv_canonical : string -> string option
> -(** Convert a non-canonical LV path like /dev/mapper/vg-lv or /dev/dm-0
> -    to a canonical one.
> -
> -    On error this raises an exception.  There are two possible non-error
> -    return cases:
> -
> -    Some lv = conversion was successful, returning the canonical LV
> -    None = input path was not an LV, it could not be made canonical *)
> diff --git a/daemon/lvm_utils.ml b/daemon/lvm_utils.ml
> new file mode 100644
> index 000000000..a891193df
> --- /dev/null
> +++ b/daemon/lvm_utils.ml
> @@ -0,0 +1,48 @@
> +(* guestfs-inspection
> + * Copyright (C) 2009-2018 Red Hat Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *)
> +
> +open Unix
> +
> +open Utils
> +
> +(* Convert a non-canonical LV path like /dev/mapper/vg-lv or /dev/dm-0
> + * to a canonical one.
> + *
> + * This is harder than it should be.  A LV device like /dev/VG/LV is
> + * really a symlink to a device-mapper device like /dev/dm-0.  However
> + * at the device-mapper (kernel) level, nothing is really known about
> + * LVM (a userspace concept).  Therefore we use a convoluted method to
> + * determine this, by listing out known LVs and checking whether the
> + * rdev (major/minor) of the device we are passed matches any of them.
> + *
> + * Note use of 'stat' instead of 'lstat' so that symlinks
are fully
> + * resolved.
> + *)
> +let lv_canonical device > +  let stat1 = stat device in
> +  let lvs = Lvm.lvs () in
> +  try
> +    Some (
> +      List.find (
> +        fun lv ->
> +          let stat2 = stat lv in
> +          stat1.st_rdev = stat2.st_rdev
> +      ) lvs
> +    )
> +  with
> +  | Not_found -> None
> diff --git a/daemon/lvm_utils.mli b/daemon/lvm_utils.mli
> new file mode 100644
> index 000000000..b25a9a706
> --- /dev/null
> +++ b/daemon/lvm_utils.mli
> @@ -0,0 +1,27 @@
> +(* guestfs-inspection
> + * Copyright (C) 2009-2018 Red Hat Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *)
> +
> +val lv_canonical : string -> string option
> +(** Convert a non-canonical LV path like /dev/mapper/vg-lv or /dev/dm-0
> +    to a canonical one.
> +
> +    On error this raises an exception.  There are two possible non-error
> +    return cases:
> +
> +    Some lv = conversion was successful, returning the canonical LV
> +    None = input path was not an LV, it could not be made canonical *)
> --
> 2.14.3
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org