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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
