[Libguestfs] [PATCH v2] v2v: factor out bootloader handling

Richard W.M. Jones rjones at redhat.com
Mon Aug 15 17:27:53 UTC 2016


On Mon, Aug 15, 2016 at 06:44:26PM +0200, Pino Toscano wrote:
> On Monday, 15 August 2016 16:26:51 CEST Richard W.M. Jones wrote:
> > On Mon, Aug 15, 2016 at 04:48:29PM +0200, Pino Toscano wrote:
> > > Create an object hierarchy to represent different bootloaders for Linux
> > > guests, moving the separate handling of grub1 and grub2 in different
> > > classes: this isolates the code for each type of bootloader together,
> > > instead of scattering it all around.
> > > 
> > > This is mostly code refactoring, with no actual behaviour change.
> > > ---
> > >  po/POTFILES-ml       |   1 +
> > >  v2v/Makefile.am      |   2 +
> > >  v2v/bootloaders.ml   | 317 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  v2v/bootloaders.mli  |  44 +++++++
> > 
> > Is this really "bootloaders"?  It's more like "linux_bootloaders",
> > unless this might one day be extended to include NTLDR.
> 
> OK, make sense -- I will rename it.
> 
> > > +class virtual bootloader = object
> > > +  method virtual name : string
> > > +  method virtual augeas_device_patterns : string list
> > > +  method virtual list_kernels : unit -> string list
> > > +  method virtual set_default_kernel : string -> unit
> > > +  method set_augeas_configuration () = false
> > > +  method virtual configure_console : unit -> unit
> > > +  method virtual remove_console : unit -> unit
> > > +  method update () = ()
> > > +end
> > 
> > I think you'll also find that you don't need to use inheritance at
> > all.  You can just declare bootloader as a class type and declare two
> > (unrelated) classes which implement the class type.  They won't be
> > able to inherit the two trivial non-virtual functions, but IMHO that's
> > a feature.
> 
> Using inheritance helps here to spot typos in methods, and also when
> implementing subclasses it'll be spotted if a method is not implemented.

I'm fairly sure they're equivalent in terms of safety actually.  I
would have to try it to be sure.

> > I still don't much see the point versus an implementation which used a
> > type Bootloader.t declared as:
> > 
> >   type t = Grub1 of <any local data grub1 needs>
> >          | Grub2 of <any local data grub2 needs>
> > 
> > (and not exposed through the mli file).  As this is to some extent my
> > personal preference, I'll let it slide.
> 
> I did this approach as well, and it turns out that:
> * every method is basically a giant "match t with Grub1 _ | Grub2 _",
>   which makes it ugly to read: this is also because the code shared
>   between one bootloader implementation and another is not really much,
>   and currently only the "remove_hd_prefix" function is
> * the implementation of a bootloader is scattered all around, so
>   looking at what is done for a bootloader means looking everywhere
> * passing a data type when calling every method is basically like
>   what is done in C to mimim OOP; hence if I have to do something that
>   resembles OOP but not actually doing it, then it will be a suboptimal
>   implementation than OO (IMHO at least)

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




More information about the Libguestfs mailing list