Libguestfs - Mar 2016 - Re: [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

What about display type?
I found there is no graphics type part showed in guest xml for Vmware esxi
guests before conversion,but after conversion by virt-v2v,the display type of
some guests changed to spice while some changed to vnc.

Best regards,
Tingting Zheng（郑婷婷）


----- Original Message -----
| From: "Richard W.M. Jones" <rjones@redhat.com>
| To: "Roman Kagan" <rkagan@virtuozzo.com>
| Cc: "Denis Lunev" <den@openvz.org>, libguestfs@redhat.com
| Sent: Wednesday, February 10, 2016 2:15:23 AM
| Subject: 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
| 
| _______________________________________________
| Libguestfs mailing list
| Libguestfs@redhat.com
| https://www.redhat.com/mailman/listinfo/libguestfs
|

On Fri, Mar 25, 2016 at 06:01:13AM -0400, Tingting Zheng
wrote:
> What about display type?
Roman's patch only applies when using --inplace, which is a mode we
disable in RHEL.
> I found there is no graphics type part showed in guest xml for
> Vmware esxi guests before conversion,but after conversion by
> virt-v2v,the display type of some guests changed to spice while some
> changed to vnc.
Those are bugs - I think we've filed several bugs in Bugzilla for
these, and fixed some of them.

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