Re: [Libguestfs] [PATCH 1/4] v2v: collect source network and video adapter types
On Tue, Feb 09, 2016 at 05:53:55PM +0300, Roman Kagan wrote:
+and s_nic_model = Source_rtl8139 | Source_e1000 | Source_virtio_net
[...]
+and source_video = Source_Cirrus | Source_QXL
As you know there could be a lot of other input devices.
At the moment the code basically ignores these (but it prints a
warning "unknown [video|network] adapter model %s ignored"). I think
we should only print warnings when that information is useful and/or
actionable for the user, which it isn't in this case. (I know we
don't always obey that rule right now, but I think it's still a good
rule ...)
So I think a better way to do it would be:
and s_nic_model =
| Source_rtl8139 | Source_e1000 | Source_virtio_net
| Source_other_nic of string
and source_video =
| Source_Cirrus | Source_QXL
| Source_other_video of string
and then change the match code to be something like:
let model =
match xpath_string "model/@type" with
| None -> None
| Some "virtio" -> Some Source_virtio_net
| Some "e1000" -> Some Source_e1000
| Some "rtl8139" -> Some Source_rtl8139
| Some model -> Some (Source_other_nic model) in
(similarly for video model). We don't throw away the NIC model if
it's something we can't handle, but we still have special cases for
virtio-net/e1000/rtl8139, so we can handle them in a type safe way.
BTW if you're wondering why I sometimes put the 'in' of 'let .. in'
on
the right hand side of the last line of code, and sometimes on a line
by itself, it's because the rule is: Attach 'in' to the last line if
what you're defining is a non-function. Put 'in' on a line on its own
if what you're defining is a function. Above is an example of a
non-function. An example of a function definition would be:
let f () =
...
in
Apart from that, the patch looks fine to me.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v