Libguestfs - Dec 2014 - Re: [PATCH v2] v2v: When picking a default kernel, favour non-debug kernels over debug kernels (RHBZ#1170073).

---
 v2v/convert_linux.ml | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index f670812..39a520c 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -49,13 +49,14 @@ type kernel_info = {
   ki_modules : string list;        (* The list of module names. *)
   ki_supports_virtio : bool;       (* Kernel has virtio drivers? *)
   ki_is_xen_kernel : bool;         (* Is a Xen paravirt kernel? *)
+  ki_is_debug : bool;              (* Is debug kernel? *)
 }
 
 let string_of_kernel_info ki -  sprintf "(%s, %s, %s, %s, %s, virtio=%b,
xen=%b)"
+  sprintf "(%s, %s, %s, %s, %s, virtio=%b, xen=%b, debug=%b)"
     ki.ki_name ki.ki_version ki.ki_arch ki.ki_vmlinuz
     (match ki.ki_initrd with None -> "None" | Some f -> f)
-    ki.ki_supports_virtio ki.ki_is_xen_kernel
+    ki.ki_supports_virtio ki.ki_is_xen_kernel ki.ki_is_debug
 
 (* The conversion function. *)
 let rec convert ~verbose ~keep_serial_console (g : G.guestfs) inspect source @@
-241,6 +242,11 @@ let rec convert ~verbose ~keep_serial_console (g : G.guestfs)
inspect source               let supports_virtio = List.mem
"virtio_net" modules in
              let is_xen_kernel = List.mem "xennet" modules in
 
+             (* If the package name is like "kernel-debug", then
it's
+              * a debug kernel.
+              *)
+             let is_debug = string_find app.G.app2_name "debug" >=
0 in
+
              Some {
                ki_app  = app;
                ki_name = name;
@@ -253,6 +259,7 @@ let rec convert ~verbose ~keep_serial_console (g :
G.guestfs) inspect source                 ki_modules = modules;
                ki_supports_virtio = supports_virtio;
                ki_is_xen_kernel = is_xen_kernel;
+               ki_is_debug = is_debug;
              }
            )
 
@@ -745,7 +752,12 @@ let rec convert ~verbose ~keep_serial_console (g :
G.guestfs) inspect source        let compare_best_kernels k1 k2          let i =
compare k1.ki_supports_virtio k2.ki_supports_virtio in
         if i <> 0 then i
-        else compare_app2_versions k1.ki_app k2.ki_app
+        else (
+          let i = compare_app2_versions k1.ki_app k2.ki_app in
+          if i <> 0 then i
+          (* Favour non-debug kernels over debug kernels (RHBZ#1170073). *)
+          else compare k2.ki_is_debug k1.ki_is_debug
+        )
       in
       let kernels = grub_kernels in
       let kernels = List.filter (fun { ki_is_xen_kernel = is_xen_kernel } ->
not is_xen_kernel) kernels in
-- 
2.1.0

On Thursday 04 December 2014 10:21:40 Richard W.M. Jones
wrote:
> ---
>  v2v/convert_linux.ml | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
> index f670812..39a520c 100644
> --- a/v2v/convert_linux.ml
> +++ b/v2v/convert_linux.ml
> @@ -49,13 +49,14 @@ type kernel_info = {
>    ki_modules : string list;        (* The list of module names. *)
>    ki_supports_virtio : bool;       (* Kernel has virtio drivers? *)
>    ki_is_xen_kernel : bool;         (* Is a Xen paravirt kernel? *)
> +  ki_is_debug : bool;              (* Is debug kernel? *)
>  }
>  
>  let string_of_kernel_info ki > -  sprintf "(%s, %s, %s, %s, %s,
virtio=%b, xen=%b)"
> +  sprintf "(%s, %s, %s, %s, %s, virtio=%b, xen=%b, debug=%b)"
>      ki.ki_name ki.ki_version ki.ki_arch ki.ki_vmlinuz
>      (match ki.ki_initrd with None -> "None" | Some f -> f)
> -    ki.ki_supports_virtio ki.ki_is_xen_kernel
> +    ki.ki_supports_virtio ki.ki_is_xen_kernel ki.ki_is_debug
>  
>  (* The conversion function. *)
>  let rec convert ~verbose ~keep_serial_console (g : G.guestfs) inspect
source > @@ -241,6 +242,11 @@ let rec convert ~verbose ~keep_serial_console
(g : G.guestfs) inspect source >               let supports_virtio = List.mem
"virtio_net" modules in
>               let is_xen_kernel = List.mem "xennet" modules in
>  
> +             (* If the package name is like "kernel-debug", then
it's
> +              * a debug kernel.
> +              *)
> +             let is_debug = string_find app.G.app2_name "debug"
>= 0 in
What about something like (simplified):
  let is_debug = string_suffix app2_name "-debug"
                 || string_suffix app2_name "-dbg" in
?

This should avoid matching "debug" in other parts of the package name
(unlikely, but possible), and also support the debug naming of packages
in Debian-based distributions.
> +
>               Some {
>                 ki_app  = app;
>                 ki_name = name;
> @@ -253,6 +259,7 @@ let rec convert ~verbose ~keep_serial_console (g :
G.guestfs) inspect source >                 ki_modules = modules;
>                 ki_supports_virtio = supports_virtio;
>                 ki_is_xen_kernel = is_xen_kernel;
> +               ki_is_debug = is_debug;
>               }
>             )
>  
> @@ -745,7 +752,12 @@ let rec convert ~verbose ~keep_serial_console (g :
G.guestfs) inspect source >        let compare_best_kernels k1 k2 >       
let i = compare k1.ki_supports_virtio k2.ki_supports_virtio in
>          if i <> 0 then i
> -        else compare_app2_versions k1.ki_app k2.ki_app
> +        else (
> +          let i = compare_app2_versions k1.ki_app k2.ki_app in
> +          if i <> 0 then i
> +          (* Favour non-debug kernels over debug kernels (RHBZ#1170073).
*)
> +          else compare k2.ki_is_debug k1.ki_is_debug
Aren't k2 andk1 swapped here?

-- 
Pino Toscano

On Thu, Dec 04, 2014 at 02:10:31PM +0100, Pino Toscano
wrote:
> On Thursday 04 December 2014 10:21:40 Richard W.M. Jones wrote:
> > ---
> >  v2v/convert_linux.ml | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
> > index f670812..39a520c 100644
> > --- a/v2v/convert_linux.ml
> > +++ b/v2v/convert_linux.ml
> > @@ -49,13 +49,14 @@ type kernel_info = {
> >    ki_modules : string list;        (* The list of module names. *)
> >    ki_supports_virtio : bool;       (* Kernel has virtio drivers? *)
> >    ki_is_xen_kernel : bool;         (* Is a Xen paravirt kernel? *)
> > +  ki_is_debug : bool;              (* Is debug kernel? *)
> >  }
> >  
> >  let string_of_kernel_info ki > > -  sprintf "(%s, %s, %s,
%s, %s, virtio=%b, xen=%b)"
> > +  sprintf "(%s, %s, %s, %s, %s, virtio=%b, xen=%b,
debug=%b)"
> >      ki.ki_name ki.ki_version ki.ki_arch ki.ki_vmlinuz
> >      (match ki.ki_initrd with None -> "None" | Some f
-> f)
> > -    ki.ki_supports_virtio ki.ki_is_xen_kernel
> > +    ki.ki_supports_virtio ki.ki_is_xen_kernel ki.ki_is_debug
> >  
> >  (* The conversion function. *)
> >  let rec convert ~verbose ~keep_serial_console (g : G.guestfs) inspect
source > > @@ -241,6 +242,11 @@ let rec convert ~verbose
~keep_serial_console (g : G.guestfs) inspect source > >               let
supports_virtio = List.mem "virtio_net" modules in
> >               let is_xen_kernel = List.mem "xennet" modules
in
> >  
> > +             (* If the package name is like "kernel-debug",
then it's
> > +              * a debug kernel.
> > +              *)
> > +             let is_debug = string_find app.G.app2_name
"debug" >= 0 in
> 
> What about something like (simplified):
>   let is_debug = string_suffix app2_name "-debug"
>                  || string_suffix app2_name "-dbg" in
> ?
> 
> This should avoid matching "debug" in other parts of the package
name
> (unlikely, but possible), and also support the debug naming of packages
> in Debian-based distributions.
OK.
> > +
> >               Some {
> >                 ki_app  = app;
> >                 ki_name = name;
> > @@ -253,6 +259,7 @@ let rec convert ~verbose ~keep_serial_console (g :
G.guestfs) inspect source > >                 ki_modules = modules;
> >                 ki_supports_virtio = supports_virtio;
> >                 ki_is_xen_kernel = is_xen_kernel;
> > +               ki_is_debug = is_debug;
> >               }
> >             )
> >  
> > @@ -745,7 +752,12 @@ let rec convert ~verbose ~keep_serial_console (g
: G.guestfs) inspect source > >        let compare_best_kernels k1 k2 >
>          let i = compare k1.ki_supports_virtio k2.ki_supports_virtio in
> >          if i <> 0 then i
> > -        else compare_app2_versions k1.ki_app k2.ki_app
> > +        else (
> > +          let i = compare_app2_versions k1.ki_app k2.ki_app in
> > +          if i <> 0 then i
> > +          (* Favour non-debug kernels over debug kernels
(RHBZ#1170073). *)
> > +          else compare k2.ki_is_debug k1.ki_is_debug
> 
> Aren't k2 andk1 swapped here?
Deliberately, because I want to favour non debug kernels.  The key is
how this function (compare_best_kernels) is used a few lines later:

   let kernels = List.sort compare_best_kernels kernels in
   let kernels = List.rev kernels (* so best is first *) in
   List.hd kernels

# let kernels = [ false; true ] ;;
val kernels : bool list = [false; true]
# List.sort (fun k1 k2 -> compare k2 k1) kernels ;;
- : bool list = [true; false]

So if kernel2 is a debug kernel and kernel1 is a non-debug kernel,
then it will order the list [kernel2, kernel1].  Because the list is
reversed, kernel1 (non-debug) wins.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW