| Message ID | 3192e26e04a4415ed01fb1a06b4e07e4ca102dbc.1498675591.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu: Default to video type=virtio for machvirt | expand |
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@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; > } > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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) { 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; }
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@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(-) -- 2.13.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list