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@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: 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
