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)
Thanks,
--
Pino Toscano
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)