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

[Peter Krempa]

On Wed, Nov 18, 2020 at 09:41:44 +0100, Michal Privoznik wrote:
> 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 at redhat.com>
> > > > > ---
> > > > >    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?

Yes, merge these as they are.

Regarding the fix for the domain caps stuff, I prefer if you add compile
time checks that each declaration of virDomainCapsEnum guards that the
corresponding enum's _LAST value is less than 32. It's not really a
runtime problem, nor anything user can fix in their configuration.