Re: [libvirt] [PATCH v2 1/5] qemu: whitelist valid video types
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(a)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