[libvirt] [PATCH 3/8] qemu: annotate some VIDEO_TYPE enum switch

[Cole Robinson]

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 at redhat.com>
>> ---
>>  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