On 01/21/2019 08:13 AM, Andrea Bolognani wrote:
On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: [...]
VIR_ENUM_IMPL(virDomainRNGModel, VIR_DOMAIN_RNG_MODEL_LAST, - "virtio"); + "virtio", + "virtio-transitional", + "virtio-non-transitional");
Since you're touching the definition, you can make it
VIR_ENUM_IMPL(virDomainRNGModel, ... "virtio-non-transitional", );
so that if and when we'll need to add another model the diff will look nicer.
Okay I'll adjust it. FWIW I like this style better but it's not well represented in domain_conf.c. I explicitly didn't make a change like this before posting because I had no idea if it would generate a complaint or not. IMO if this is the recommended style then we should fix all instances in one sweep and add a syntax-check for it
[...]
@@ -1122,6 +1124,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { {"virtio-net-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_NET_NON_TRANSITIONAL}, {"vhost-scsi-pci-transitional", QEMU_CAPS_DEVICE_VHOST_SCSI_TRANSITIONAL}, {"vhost-scsi-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_SCSI_NON_TRANSITIONAL}, + {"virtio-rng-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_RNG_TRANSITIONAL}, + {"virtio-rng-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_RNG_NON_TRANSITIONAL}, };
Same comments as for the other capabilities.
[...]
@@ -5990,7 +5995,7 @@ qemuBuildRNGDevStr(const virDomainDef *def, { virBuffer buf = VIR_BUFFER_INITIALIZER;
- if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || + if (dev->model == VIR_DOMAIN_RNG_MODEL_VIRTIO && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("this qemu doesn't support RNG device type '%s'"),
Idea for a possible follow-up series: wouldn't it be great if we centralized capabilities checking for VirtIO devices in a function that can be called during device validation? Delaying such checks until command line generation is not optimal, and neither is having some of them centralized and some of them sprinkled around the various qemuBuild*DevStr() functions...
Yeah the validation checks sprinkled all around are a pain, I hit it quite a few times in this series but I tried to resist the temptation of the rabbit hole...
[...]
@@ -2290,8 +2295,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
/* VirtIO RNG */ for (i = 0; i < def->nrngs; i++) { - if (def->rngs[i]->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || - !virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) + if (!virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) continue;
I'm not convinced you can just remove the model check here... Shouldn't you extend it to include MODEL_VIRTIO_TRANSITIONAL and MODEL_VIRTIO_NON_TRANSITIONAL instead?
It seemed redundant... the only enum value for rng model prior to this patch was MODEL_VIRTIO, so any rng device here will by definition be MODEL_VIRTIO. Same after this patch, but plus 2 more virtio models. It's future proof if a non-PCI rng device model is added, but it causes more work when a PCI rng device model is added, so it's a toss up Elsewhere in this function, some devices use a whitelist approach (watchdog, memballoon although it's equally redundant), some use a blacklist approach (sound cards, controllers, basically everything else). By deleting the line here it essentially moves rng address allocation to a blacklist approach. So AFAICT it's safe to do, but I can move the change to a separate patch and add a comment, similar to the comment for FS devices near here Thanks, Cole