diff mbox series

[v2,1/5] qemu: whitelist valid video types

Message ID c2664a0600d29fd41130a6a196f78b879d93bd20.1503846114.git.crobinso@redhat.com
State New
Headers show
Series qemu: Default to video type=virtio for machvirt | expand

Commit Message

Cole Robinson Aug. 27, 2017, 3:04 p.m. UTC 
Rather than require an explicit blacklist that needs to be extended
for every new VIDEO_TYPE

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 src/qemu/qemu_domain.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

-- 
2.13.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Pavel Hrdina Aug. 28, 2017, 7:56 a.m. UTC | #1 
On Sun, Aug 27, 2017 at 11:04:38AM -0400, Cole Robinson wrote:
> Rather than require an explicit blacklist that needs to be extended

> for every new VIDEO_TYPE

> 

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

>  src/qemu/qemu_domain.c | 13 +++++--------

>  1 file changed, 5 insertions(+), 8 deletions(-)


NACK, we prefer to listing all possible values for typed switch.  It
forces a contributor to look at all places where that switch is used
in order to consider whether that place should be updated or not.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Aug. 28, 2017, 2:44 p.m. UTC | #2 
On 08/28/2017 03:56 AM, Pavel Hrdina wrote:
> On Sun, Aug 27, 2017 at 11:04:38AM -0400, Cole Robinson wrote:

>> Rather than require an explicit blacklist that needs to be extended

>> for every new VIDEO_TYPE

>>

>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

>> ---

>>  src/qemu/qemu_domain.c | 13 +++++--------

>>  1 file changed, 5 insertions(+), 8 deletions(-)

> 

> NACK, we prefer to listing all possible values for typed switch.  It

> forces a contributor to look at all places where that switch is used

> in order to consider whether that place should be updated or not.

> 


Thanks for the review. I don't really see the benefit of listing all
VIDEO_TYPE in this case. If a new TYPE is added, either it's:

1) Something qemu supports and the developer is adding qemu support for it.
There's no way they can miss extending this switch to whitelist the new type,
since otherwise their qemu XML will be rejected at parse time. It's the first
functional thing in src/qemu they have to change

2) Something being added for a non-qemu driver. Maybe qemu supports it, maybe
it doesn't, but regardless the developer isn't on the hook for implementing it
for qemu. In this case, adding the new VIDEO_TYPE to the blacklist is slightly
better code documentation, but it adds no runtime benefits over the 'default:'
case. Plus there's potential issues if the user forgets to add the new TYPE
(like TYPE_GOP currently but the impact is small, start time vs parse time
failure). We could enforce the switch type checking with (virDomainVideoType)
cast but that could lead to build breakage if the dev isn't compiling the qemu
driver.

In some cases I think it makes sense to list all enum values but IMO this
isn't one of them.

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Aug. 30, 2017, 7:14 p.m. UTC | #3 
On 08/28/2017 03:56 AM, Pavel Hrdina wrote:
> On Sun, Aug 27, 2017 at 11:04:38AM -0400, Cole Robinson wrote:

>> Rather than require an explicit blacklist that needs to be extended

>> for every new VIDEO_TYPE

>>

>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

>> ---

>>  src/qemu/qemu_domain.c | 13 +++++--------

>>  1 file changed, 5 insertions(+), 8 deletions(-)

> 

> NACK, we prefer to listing all possible values for typed switch.  It

> forces a contributor to look at all places where that switch is used

> in order to consider whether that place should be updated or not.

> 


This patch isn't necessary anyways, I can drop it and squash this into patch
#2, ACK to that? (if so I'll push after the release)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8f93018d8..eb058c57c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3017,6 +3017,7 @@ qemuDomainDefValidateVideo(const virDomainDef *def)
         case VIR_DOMAIN_VIDEO_TYPE_XEN:
         case VIR_DOMAIN_VIDEO_TYPE_VBOX:
         case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
+        case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("video type '%s' is not supported with QEMU"),
                            virDomainVideoTypeToString(video->type));




Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Pavel Hrdina Aug. 30, 2017, 7:42 p.m. UTC | #4 
On Mon, Aug 28, 2017 at 10:44:43AM -0400, Cole Robinson wrote:
> On 08/28/2017 03:56 AM, Pavel Hrdina wrote:

> > On Sun, Aug 27, 2017 at 11:04:38AM -0400, Cole Robinson wrote:

> >> Rather than require an explicit blacklist that needs to be extended

> >> for every new VIDEO_TYPE

> >>

> >> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> >> ---

> >>  src/qemu/qemu_domain.c | 13 +++++--------

> >>  1 file changed, 5 insertions(+), 8 deletions(-)

> > 

> > NACK, we prefer to listing all possible values for typed switch.  It

> > forces a contributor to look at all places where that switch is used

> > in order to consider whether that place should be updated or not.

> > 

> 

> Thanks for the review. I don't really see the benefit of listing all

> VIDEO_TYPE in this case. If a new TYPE is added, either it's:


Sorry for the late reply, I was solving the chardev reconnect issues.

> 1) Something qemu supports and the developer is adding qemu support for it.

> There's no way they can miss extending this switch to whitelist the new type,

> since otherwise their qemu XML will be rejected at parse time. It's the first

> functional thing in src/qemu they have to change


That's true but having the typed switch that forces you to cover all
values without default would save you the trouble of building libvirt,
starting the daemon and defining new domain.  It would fail while
building.  Yes, most of the developers adding such feature would
probably extend that switch but still if you forget to do it this would
help you realize that right away.

> 2) Something being added for a non-qemu driver. Maybe qemu supports it, maybe

> it doesn't, but regardless the developer isn't on the hook for implementing it

> for qemu. In this case, adding the new VIDEO_TYPE to the blacklist is slightly

> better code documentation, but it adds no runtime benefits over the 'default:'

> case. Plus there's potential issues if the user forgets to add the new TYPE

> (like TYPE_GOP currently but the impact is small, start time vs parse time

> failure). We could enforce the switch type checking with (virDomainVideoType)

> cast but that could lead to build breakage if the dev isn't compiling the qemu

> driver.


So instead of using default I would actually prefer casting the
video->type to virDomainVideoType so if forces you to always consider
this place.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Pavel Hrdina Aug. 30, 2017, 7:46 p.m. UTC | #5 
On Wed, Aug 30, 2017 at 03:14:40PM -0400, Cole Robinson wrote:
> On 08/28/2017 03:56 AM, Pavel Hrdina wrote:

> > On Sun, Aug 27, 2017 at 11:04:38AM -0400, Cole Robinson wrote:

> >> Rather than require an explicit blacklist that needs to be extended

> >> for every new VIDEO_TYPE

> >>

> >> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> >> ---

> >>  src/qemu/qemu_domain.c | 13 +++++--------

> >>  1 file changed, 5 insertions(+), 8 deletions(-)

> > 

> > NACK, we prefer to listing all possible values for typed switch.  It

> > forces a contributor to look at all places where that switch is used

> > in order to consider whether that place should be updated or not.

> > 

> 

> This patch isn't necessary anyways, I can drop it and squash this into patch

> #2, ACK to that? (if so I'll push after the release)


ACK to that, I was about to suggest it :).

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox series

Patch

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2c77a6442..1c6b1ba79 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3014,20 +3014,17 @@  qemuDomainDefValidateVideo(const virDomainDef *def)
         video = def->videos[i];
 
         switch (video->type) {
-        case VIR_DOMAIN_VIDEO_TYPE_XEN:
-        case VIR_DOMAIN_VIDEO_TYPE_VBOX:
-        case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("video type '%s' is not supported with QEMU"),
-                           virDomainVideoTypeToString(video->type));
-            return -1;
         case VIR_DOMAIN_VIDEO_TYPE_VGA:
         case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
         case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
         case VIR_DOMAIN_VIDEO_TYPE_QXL:
         case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
-        case VIR_DOMAIN_VIDEO_TYPE_LAST:
             break;
+        default:
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("video type '%s' is not supported with QEMU"),
+                           virDomainVideoTypeToString(video->type));
+            return -1;
         }
 
         if (!video->primary &&