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

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.

>> * 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.