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

[Cole Robinson]

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.

>>
>>   <model type='idontexist'/>
>>
>> qemu would turn that into
>>
>>   -device idontexist,...
>>
>> That behavior is untouched by this series. Unwinding it will take
>> some thought. For starters would be populating the enum with any
>> qemu/xen network model that a user could realistically have a working
>> config with. Then maybe tainting the VM if modelstr is used. Then
>> after some time the final round could be causing domains to fail to
>> start, but not fail to parse, so they don't disappear on upgrade but
>> still break in an obvious way that will generate
>> complaints/bugreports. But we should agree on a plan for it.
> 
> Yes, I think we should mark the guest as "tainted" as that ensures
> a log message in the per-QEMU log file. We should also ensure we
> use VIR_WARN in libvirtd log too i guess.
> 
> We'd want to keep this loose acceptance for at least a year,
> possibly two before we consider removing it. 
> 
>> Other caveats:
>> * vz and bhyve drivers are not compile tested
>>
>> * 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.

- Cole