[libvirt] [PATCH 00/10] RFC: conf: partially net model enum conversion

[Daniel P. Berrangé]

On Tue, Jan 22, 2019 at 10:52:25AM -0500, Cole Robinson wrote:
> On 01/22/2019 06:55 AM, Daniel P. Berrangé wrote:
> > On Fri, Jan 18, 2019 at 06:05:18PM -0500, Cole Robinson wrote:
> >> This series is base on my virtio-transitional work, since it touches
> >> a lot of the same code:
> >> https://www.redhat.com/archives/libvir-list/2019-January/msg00593.html
> >>
> >> This series partially converts the net->model value from a string
> >> to an enum. We wrap the existing ->model string in accessor functions,
> >> rename it to ->modelstr, add a ->model enum, and convert internal
> >> driver usage bit by bit. At the end, all driver code that is acting
> >> on specific network model values is comparing against an enum, not
> >> a string.
> >>
> >> This is only partial because of xen/libxl/xm and qemu drivers, which
> >> if they don't know anything particular about the model string will
> >> just place it on the qemu command line/xen config and see what happens.
> >> So basically if I were to pass in
> > 
> > Xen is probably quite easy to deal with as it supports far fewer
> > arches than the qemu driver. On x86 we need to handle the usual
> > rtl8139, e100, ne2k, etc for Xen fully virt. I think for other
> > Xen non-x86 arches, it might be reasonable to only care about
> > net-front even for fully-virt given that people using Xen with
> > hardware acceleration won't want a slow NIC.
> > 
> 
> Okay sounds good. I'll save that for a follow up series after doing some
> research/
> 
> > QEMU is the really hard one as it has a huge set of arches and
> > people using QEMU in TCG mode conceivably care about all the
> > random NIC models no matter how slow & awful they are, because
> > TCG is already slow & awful.
> > 
> 
> This is probably not as bad as you suggest. I believe most tcg qemu
> variants can't even handle -device <netmodel> syntax, but require old
> style -net nic magic to enable platform devices. Internally in libvirt
> we have a config whitelist for using that old style syntax, and it only
> covers a small number, basically arm32/arm64 with non-virtio nic:
> 
> bool
> qemuDomainSupportsNicdev(virDomainDefPtr def,
>                          virDomainNetDefPtr net)
> {
>     /* non-virtio ARM nics require legacy -net nic */
>     if (((def->os.arch == VIR_ARCH_ARMV6L) ||
>         (def->os.arch == VIR_ARCH_ARMV7L) ||
>         (def->os.arch == VIR_ARCH_AARCH64)) &&
>         net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
>         net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
>         return false;
> 
>     return true;
> }
> 
> So if we determine a tcg config requires -net nic, and it doesn't meet
> the above rules, it doesn't work with libvirt anyways.

Oh that's a very good point - so we really just need to look at
'qemu-system-XXXX -device help' output for each XXXX target and
look for which devices are NICs.


> >> * vmx and virtualbox drivers previously would do a case insensitive
> >>   compare on the model value passed in via the user XML. Current
> >>   patches don't preserve that. I don't know how much it matters
> >>   for these drivers for reading fresh XML vs roundtrip XML from
> >>   converted native formats (which _will_ correct the case issue).
> >>   If we want to fix this we will need to tolower() the modelstr
> >>   value in the XML parser. I think there's a gnulib function for
> >>   it but I haven't explored it deeply
> > 
> > I guess if we're going to be serious about maintaining compat, we
> > should accept the insensitive strings, at the very least for a
> > transitional period.
> > 
> > Normally all our enums would be 100% lowercase, but in the vbox
> > driver you're adding model names which are mixed-case, because
> > that's what vbox would previously output. I think you are right
> > to preserve that mixed case despite it not being our normal
> > practice, because round-tripped XML is important.
> > 
> > I fear there there could well be people feeding in vbox XML that
> > uses all-lowercase for these vbox models based on normal libvirt
> > precedent though.  This re-inforces the idea that we should allow
> > a case-insensitive match when parsing, and perhaps log a warning
> > if people use the non-preferred case.  Perhaps after a year or
> > two we could drop the case-insensitive match, but it would not
> > be a huge burden to just carry it forever.
> > 
> 
> Okay I'll explore the tolower() option. I guess then that will imply
> that all enum strings are lowercase which means generated vbox XML will
> be different now, but that seems like a minor issue if XML input compat
> is maintained.

We shouldn't need to change output.  I was just thinking that you
could do something like this when parsing to an enum:

   model = virDomainNetModelFromString(modelstr)
   if model < 0
       lowermodelstr = tolower(modelstr)
       model = virDomainNetModelFromString(lowermodelstr)
       if model < 0
            return -1;
       VIR_WARN("Model string '%s' uses unexpected case, pleae use '%s",
                lowermodelstr, virDomainNetModelToString(model));

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|