[PATCH 5/5] qemu_validate: Deduplicate code for graphics type check
Michal Privoznik
mprivozn at redhat.com
Wed Nov 18 08:41:44 UTC 2020
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?
Michal
More information about the libvir-list
mailing list