On 07/18/2017 06:24 PM, John Ferlan wrote:
>
>
> On 06/28/2017 02:49 PM, Cole Robinson wrote:
>> For the ram/vram monitor wrappers, just add a default: clause...
>> seems like it should be rarely extended so this saves every committer
>> from needing to update
>>
>> For the validation switch, fill in the missing values
>>
>> Signed-off-by: Cole Robinson <crobinso@xxxxxxxxxx>
>> ---
>> src/qemu/qemu_domain.c | 3 ++-
>> src/qemu/qemu_monitor_json.c | 16 ++++------------
>> src/qemu/qemu_process.c | 7 ++-----
>> 3 files changed, 8 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 90f489840..ac1bc1a1e 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -2950,10 +2950,11 @@ qemuDomainDefValidateVideo(const virDomainDef *def)
>> for (i = 0; i < def->nvideos; i++) {
>> video = def->videos[i];
>>
>> - switch (video->type) {
>> + switch ((virDomainVideoType) video->type) {
>
> My recollection is when this is typecast or the @type was typed as the
> enum, then the switch needed every case of the enum to be listed.
>
> Whereas, when the @type was an int, then using 'default:' was possible
> if one didn't want to provide every possible combination.
>
It seems to work a bit differently:
- If @type is int, no checking is done.
- If @type is explicit, gcc checks that either all values are listed,
or a default: clause is present
> Still, I believe more recent changes have always favored the list every
> possible case, even if they do nothing rather than using default:
>
> Is there any special reason to not list every case option? If not, I'd
> prefer _virDomainVideoDef change @type from int to virDomainVideoType if
> only to avoid this particular type situation in the future.
>
That said I think it is beneficial to make the VideoDef change and adjust all
the users to add an explicit 'default:' if it makes sense. There aren't many
cases I can think of outside generic domain_conf code where explicitly listing
every VIDEO_TYPE makes sense IMO. There's a bunch of similar cases like that
in qemu_domain_address.c but I wonder if that is actually preventing bugs from
being added or just saving developers a few minutes hunting through the code...
Anyways I'll side step this discussion in my v2 by converting the qemu
validation switch to a whitelist approach which makes more sense anyways, and
just skipping the (virDomainVideoType) annotation
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
