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

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