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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
