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

[John Ferlan]

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.

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.

We may not add new types that often, but as shown by this particular
switch, GOP (added in 3.2) wasn't added into the list and the default
here is not an error. I'd almost say in this particular switch/case that
this one change could be a separate patch (e.g. bug fix).

If it's that rare, then it shouldn't cause too much hassle for new video
types, but does force future changes to consider all the places where
adding a new video type that need to change.

I'd rather see future adjustment be forced to decide where something
applies or does not apply. However, I'm willing to be convinced otherwise...

John


>          case VIR_DOMAIN_VIDEO_TYPE_XEN:
>          case VIR_DOMAIN_VIDEO_TYPE_VBOX:
>          case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
> +        case VIR_DOMAIN_VIDEO_TYPE_GOP:
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                             _("video type '%s' is not supported with QEMU"),
>                             virDomainVideoTypeToString(video->type));
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 5ddc09ca6..2afc03329 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1565,7 +1565,7 @@ qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon,
>          {0}
>      };
>  
> -    switch (video->type) {
> +    switch ((virDomainVideoType) video->type) {
>      case VIR_DOMAIN_VIDEO_TYPE_VGA:
>          if (qemuMonitorJSONGetObjectProperty(mon, path, "vgamem_mb", &prop) < 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1608,10 +1608,7 @@ qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon,
>          }
>          video->vram = prop.val.ul * 1024;
>          break;
> -    case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
> -    case VIR_DOMAIN_VIDEO_TYPE_XEN:
> -    case VIR_DOMAIN_VIDEO_TYPE_VBOX:
> -    case VIR_DOMAIN_VIDEO_TYPE_LAST:
> +    default:
>          break;
>      }
>  
> @@ -1635,7 +1632,7 @@ qemuMonitorJSONUpdateVideoVram64Size(qemuMonitorPtr mon,
>          {0}
>      };
>  
> -    switch (video->type) {
> +    switch ((virDomainVideoType) video->type) {
>      case VIR_DOMAIN_VIDEO_TYPE_QXL:
>          if (video->vram64 != 0) {
>              if (qemuMonitorJSONGetObjectProperty(mon, path,
> @@ -1648,12 +1645,7 @@ qemuMonitorJSONUpdateVideoVram64Size(qemuMonitorPtr mon,
>              video->vram64 = prop.val.ul * 1024;
>          }
>          break;
> -    case VIR_DOMAIN_VIDEO_TYPE_VGA:
> -    case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
> -    case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
> -    case VIR_DOMAIN_VIDEO_TYPE_XEN:
> -    case VIR_DOMAIN_VIDEO_TYPE_VBOX:
> -    case VIR_DOMAIN_VIDEO_TYPE_LAST:
> +    default:
>          break;
>      }
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d669dfb32..fb6e2c82b 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2605,7 +2605,7 @@ qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver,
>      for (i = 0; i < vm->def->nvideos; i++) {
>          video = vm->def->videos[i];
>  
> -        switch (video->type) {
> +        switch ((virDomainVideoType) video->type) {
>          case VIR_DOMAIN_VIDEO_TYPE_VGA:
>              if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VGA_VGAMEM)) {
>                  if (qemuMonitorUpdateVideoMemorySize(priv->mon, video, "VGA") < 0)
> @@ -2642,10 +2642,7 @@ qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver,
>                      goto error;
>              }
>              break;
> -        case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
> -        case VIR_DOMAIN_VIDEO_TYPE_XEN:
> -        case VIR_DOMAIN_VIDEO_TYPE_VBOX:
> -        case VIR_DOMAIN_VIDEO_TYPE_LAST:
> +        default:
>              break;
>          }
>  
>