On 01/22/2019 07:03 AM, Andrea Bolognani wrote:
> On Mon, 2019-01-21 at 19:01 -0500, Cole Robinson wrote:
>> 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
>
> I'm not sure I would qualify this style as "recommended", but I
> certainly prefer it and there are instances of the pattern around
> the codebase, so I'll switch to it whenever I happen to be touching
> a VIR_ENUM_IMPL() call. I haven't had any trouble getting such
> changes past reviewers so far :)
>
Noted, I'll do the same
>>> [...]
>>>> @@ -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
>
> When in doubt, I usually go for the safest / more explicit version.
>
I mean, it's arguably safer and more explicit to duplicate every
DomainValidate device whitelist check in qemu_command.c too, but it
comes at the cost of more code and increased developer effort when
extending related code. In this particular case it took me extra time
when writing this code to figure out why the new rng models were not
generating the expected addresses.
How about I add a prep patch that moves the RNG model check from
qemu_command.c into the qemu*Validate path, that way we are enforcing
earlier that no non-virtio RNG model can even enter this part of the
qemu driver.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
