Re: [PATCH 5/5] qemu_validate: Deduplicate code for graphics type check

On 11/18/20 9:32 AM, Peter Krempa wrote: > On Wed, Nov 18, 2020 at 09:12:26 +0100, Michal Privoznik wrote: > > On 11/18/20 8:59 AM, Peter Krempa wrote: > > > On Tue, Nov 17, 2020 at 12:28:27 +0100, Michal Privoznik wrote: > > > > Similarly to previous commits, we can utilize domCaps to check if > > > > graphics type is supported. > > > > > > > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > > > > --- > > > > src/qemu/qemu_capabilities.c | 2 +- > > > > src/qemu/qemu_capabilities.h | 3 +++ > > > > src/qemu/qemu_validate.c | 40 ++++++++++++------------------------ > > > > 3 files changed, 17 insertions(+), 28 deletions(-) > > > > > > [...] > > > > > > > @@ -3903,15 +3892,12 @@ qemuValidateDomainDeviceDefGraphics(const virDomainGraphicsDef *graphics, > > > > } > > > > break; > > > > + > > > > + case VIR_DOMAIN_GRAPHICS_TYPE_VNC: > > > > case VIR_DOMAIN_GRAPHICS_TYPE_RDP: > > > > case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: > > > > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > > > > - _("unsupported graphics type '%s'"), > > > > - virDomainGraphicsTypeToString(graphics->type)); > > > > - return -1; > > > > case VIR_DOMAIN_GRAPHICS_TYPE_LAST: > > > > - default: > > > > - return -1; > > > > + break; > > > > > > Removing 'default: ' is not necessary once you use proper type for the > > > variable in the switch statement, which is our usual approach. > > > > I guess I don't need to typecast ->type since it's already stored as > > virDomainGraphicsType in the struct. > > > > > > > > The default and _LAST case should use virReportEnumRangeError. > > > > > > > > > > I'm not sure it's adding useful code. In this very patch I'm adding a check > > that rejects unknown values for ->type and thus I don't see a way how the > > control could hit any of these 'case', or 'default'. > > You are right it won't . > > A drawback of using the capability code is that > it relies on converting the type to a bit array stored in "unsigned int" > without any overflow safeguards. Once a device model or type enum > reaches 33 entries, the code will start failing without any obvious > sign. > > virDomainCapsEnumSet which is internally used by the VIR_DOMAIN_CAPS_ENUM_SET > which is used by virQEMUCapsFillDomainDeviceGraphicsCaps to fill the > bitmap catches overflow but the callers neglect to propagate it. > Alright, so how about I post a follow up patch that checks for retval of virDomainCapsEnumSet() and merge this patch as is? Would that work for you?