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@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? Michal