Re: [libvirt] [PATCH 3/8] qemu: annotate some VIDEO_TYPE enum switch
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(a)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