On 01/21/2019 11:59 AM, Andrea Bolognani wrote: > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > [...] >> +typedef enum { >> + VIR_DOMAIN_CONTROLLER_MODEL_VIRTIO_SERIAL_DEFAULT = -1, >> + VIR_DOMAIN_CONTROLLER_MODEL_VIRTIO_SERIAL_VIRTIO, >> + VIR_DOMAIN_CONTROLLER_MODEL_VIRTIO_SERIAL_VIRTIO_TRANSITIONAL, >> + VIR_DOMAIN_CONTROLLER_MODEL_VIRTIO_SERIAL_VIRTIO_NON_TRANSITIONAL, >> + >> + VIR_DOMAIN_CONTROLLER_MODEL_VIRTIO_SERIAL_LAST >> +} virDomainControllerModelVirtioSerial; > > Do we even need MODEL_VIRTIO_SERIAL_DEFAULT here? Honest question :) > The DEFAULT = -1 is the pattern used by other controller models, and I believe it's kinda enforced by ControllerDefNew which sets cont->model = -1. It might not serve any purpose in this particular case besides consistency. Working in this area made me explore enum DEFAULT/NONE naming a bit and there's definitely a lot of room for making things consistent. controller models are the oddball here with the -1 value but I think that's to do with some old XML migration compat that can be unwound in a different way. Another task for another day > [...] >> @@ -670,6 +670,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, >> break; >> >> case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: >> + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_VIRTIO_SERIAL_VIRTIO_TRANSITIONAL) >> + return pciFlags; >> return virtioFlags; > > I'd prefer to see the kind of switch statement you added for > VIR_DOMAIN_DEVICE_VSOCK here as well. > Okay I'll try to expand that pattern out to other patches too - Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list