Libguestfs - Jul 2016 - [PATCH 2/2] v2v: fix priority of match in configure_kernel_modules

Hi,

due to a broken discover_modpath function, and to a wrong block for a
match statement, the modprobe options for virtio/SCSI blocks where not
written at all.

This series provides a small refactor, and the fixes.

Thanks,
-- 

Pino Toscano (2):
  v2v: fix and implify the internal Convert_linux:discover_modpath
  v2v: fix priority of match in configure_kernel_modules

 v2v/convert_linux.ml | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

-- 
2.5.5

First check for the existence of the directory /etc/modprobe.d, in case
using a file under it; this also skips all the other checks, since they
are not needed at all.  Also /etc/modprobe.d exists on recent Linux
versions, so let's give priority to the more common methods.
When /etc/modprobe.d does not exist, check for the file to edit using a
single list of possible files, now in order of priority, where the first
find is used without checking further for the rest.

Also, make sure all the returned paths are absolute: they are used in
Augeas paths later on, so relative paths will not do anything useful.
---
 v2v/convert_linux.ml | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index cadfbb5..0296844 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -1219,28 +1219,18 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps
   and discover_modpath ()      (* Find what /etc/modprobe.conf is called today.
*)
-    let modpath = ref "" in
-
-    (* Note that we're checking in ascending order of preference so
-     * that the last discovered method will be chosen.
-     *)
-    List.iter (
-      fun file ->
-        if g#is_file ~followsymlinks:true file then
-          modpath := file
-    ) [ "/etc/conf.modules"; "/etc/modules.conf" ];
-
-    if g#is_file ~followsymlinks:true "/etc/modprobe.conf" then
-      modpath := "modprobe.conf";
-
-    if g#is_dir ~followsymlinks:true "/etc/modprobe.d" then
+    if g#is_dir ~followsymlinks:true "/etc/modprobe.d" then (
       (* Create a new file /etc/modprobe.d/virt-v2v-added.conf. *)
-      modpath := "modprobe.d/virt-v2v-added.conf";
-
-    if !modpath = "" then
-      error (f_"unable to find any valid modprobe configuration file such
as /etc/modprobe.conf");
+      "/etc/modprobe.d/virt-v2v-added.conf"
+    ) else (
+      (* List of methods, in order of preference. *)
+      let paths = [ "/etc/modprobe.conf";
"/etc/modules.conf"; "/etc/conf.modules" ] in
 
-    !modpath
+      try
+        List.find (g#is_file ~followsymlinks:true) paths
+      with Not_found ->
+        error (f_"unable to find any valid modprobe configuration file
such as /etc/modprobe.conf");
+    )
 
   and remap_block_devices block_type      (* This function's job is to
iterate over boot configuration
-- 
2.5.5

This makes sure that in the internal configure_kernel_modules function,
for virtio or SCSI block types:
a) the warnings about leftover Xen modules are printed
b) the changes in Augeas are saved
---
 v2v/convert_linux.ml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index 0296844..a5ba8dd 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -1147,7 +1147,7 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps
     (* Update 'alias scsi_hostadapter ...' *)
     let paths = augeas_modprobe ". =~
regexp('scsi_hostadapter.*')" in
-    match block_type with
+    (match block_type with
     | Virtio_blk | Virtio_SCSI ->
       let block_module          match block_type with
@@ -1180,7 +1180,7 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps      | IDE ->
       (* There is no scsi controller in an IDE guest. *)
       List.iter (fun path -> ignore (g#aug_rm path)) (List.rev paths)
-    ;
+    );
 
     (* Display a warning about any leftover Xen modules which we
      * haven't converted.  These are likely to cause an error when
-- 
2.5.5

On Fri, Jul 01, 2016 at 05:52:00PM +0200, Pino Toscano
wrote:
> First check for the existence of the directory /etc/modprobe.d, in case
> using a file under it; this also skips all the other checks, since they
> are not needed at all.  Also /etc/modprobe.d exists on recent Linux
> versions, so let's give priority to the more common methods.
> When /etc/modprobe.d does not exist, check for the file to edit using a
> single list of possible files, now in order of priority, where the first
> find is used without checking further for the rest.
> 
> Also, make sure all the returned paths are absolute: they are used in
> Augeas paths later on, so relative paths will not do anything useful.
> ---
>  v2v/convert_linux.ml | 30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
> index cadfbb5..0296844 100644
> --- a/v2v/convert_linux.ml
> +++ b/v2v/convert_linux.ml
> @@ -1219,28 +1219,18 @@ let rec convert ~keep_serial_console (g :
G.guestfs) inspect source rcaps >
>    and discover_modpath () >      (* Find what /etc/modprobe.conf is
called today. *)
> -    let modpath = ref "" in
> -
> -    (* Note that we're checking in ascending order of preference so
> -     * that the last discovered method will be chosen.
> -     *)
> -    List.iter (
> -      fun file ->
> -        if g#is_file ~followsymlinks:true file then
> -          modpath := file
> -    ) [ "/etc/conf.modules"; "/etc/modules.conf" ];
> -
> -    if g#is_file ~followsymlinks:true "/etc/modprobe.conf" then
> -      modpath := "modprobe.conf";
> -
> -    if g#is_dir ~followsymlinks:true "/etc/modprobe.d" then
> +    if g#is_dir ~followsymlinks:true "/etc/modprobe.d" then (
>        (* Create a new file /etc/modprobe.d/virt-v2v-added.conf. *)
> -      modpath := "modprobe.d/virt-v2v-added.conf";
> -
> -    if !modpath = "" then
> -      error (f_"unable to find any valid modprobe configuration file
such as /etc/modprobe.conf");
> +      "/etc/modprobe.d/virt-v2v-added.conf"
> +    ) else (
> +      (* List of methods, in order of preference. *)
> +      let paths = [ "/etc/modprobe.conf";
"/etc/modules.conf"; "/etc/conf.modules" ] in
>  
> -    !modpath
> +      try
> +        List.find (g#is_file ~followsymlinks:true) paths
> +      with Not_found ->
> +        error (f_"unable to find any valid modprobe configuration
file such as /etc/modprobe.conf");
> +    )
>  
>    and remap_block_devices block_type >      (* This function's job
is to iterate over boot configuration
OK this looks like a natural rewrite of the existing function, except
always returning an absolute path instead of a mix.

ACK.

Rich.

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

On Fri, Jul 01, 2016 at 05:52:01PM +0200, Pino Toscano
wrote:
> This makes sure that in the internal configure_kernel_modules function,
> for virtio or SCSI block types:
> a) the warnings about leftover Xen modules are printed
> b) the changes in Augeas are saved
Best to note in the commit message that this fixes
commit ee0219148352695ddf1102a426dcfe4e3be55fba.

ACK.

Rich.
>  v2v/convert_linux.ml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
> index 0296844..a5ba8dd 100644
> --- a/v2v/convert_linux.ml
> +++ b/v2v/convert_linux.ml
> @@ -1147,7 +1147,7 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps >
>      (* Update 'alias scsi_hostadapter ...' *)
>      let paths = augeas_modprobe ". =~
regexp('scsi_hostadapter.*')" in
> -    match block_type with
> +    (match block_type with
>      | Virtio_blk | Virtio_SCSI ->
>        let block_module >          match block_type with
> @@ -1180,7 +1180,7 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps >      | IDE ->
>        (* There is no scsi controller in an IDE guest. *)
>        List.iter (fun path -> ignore (g#aug_rm path)) (List.rev paths)
> -    ;
> +    );
>  
>      (* Display a warning about any leftover Xen modules which we
>       * haven't converted.  These are likely to cause an error when
> -- 
> 2.5.5
> 
> _______________________________________________
> 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
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top