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(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 7745c968de..0efe5462b5 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5976,7 +5976,7 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps,
}
-static void
+void
virQEMUCapsFillDomainDeviceGraphicsCaps(virQEMUCapsPtr qemuCaps,
virDomainCapsDeviceGraphicsPtr dev)
{
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 0b28c9b635..1194f90140 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -739,6 +739,9 @@ int virQEMUCapsFillDomainCaps(virQEMUCapsPtr qemuCaps,
virFirmwarePtr *firmwares,
size_t nfirmwares);
+void virQEMUCapsFillDomainDeviceGraphicsCaps(virQEMUCapsPtr qemuCaps,
+ virDomainCapsDeviceGraphicsPtr dev);
+
void virQEMUCapsFillDomainDeviceVideoCaps(virQEMUCapsPtr qemuCaps,
virDomainCapsDeviceVideoPtr dev);
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 7e2fe81e08..6ddef3de35 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3752,12 +3752,6 @@ qemuValidateDomainDeviceDefSPICEGraphics(const virDomainGraphicsDef *graphics,
virDomainGraphicsListenDefPtr glisten = NULL;
int tlsPort = graphics->data.spice.tlsPort;
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("spice graphics are not supported with this QEMU"));
- return -1;
- }
-
glisten = virDomainGraphicsGetListen((virDomainGraphicsDefPtr)graphics, 0);
if (!glisten) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -3823,9 +3817,19 @@ qemuValidateDomainDeviceDefGraphics(const virDomainGraphicsDef *graphics,
virQEMUDriverPtr driver,
virQEMUCapsPtr qemuCaps)
{
+ virDomainCapsDeviceGraphics graphicsCaps = { 0 };
bool have_egl_headless = false;
size_t i;
+ virQEMUCapsFillDomainDeviceGraphicsCaps(qemuCaps, &graphicsCaps);
+
+ if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(graphicsCaps.type, graphics->type)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("%s graphics are not supported with this QEMU"),
+ virDomainGraphicsTypeToString(graphics->type));
+ return -1;
+ }
+
for (i = 0; i < def->ngraphics; i++) {
if (def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS) {
have_egl_headless = true;
@@ -3838,13 +3842,6 @@ qemuValidateDomainDeviceDefGraphics(const virDomainGraphicsDef *graphics,
* supported by QEMU.
*/
if (have_egl_headless) {
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_EGL_HEADLESS)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("egl-headless display is not supported with this "
- "QEMU binary"));
- return -1;
- }
-
if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS &&
graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
@@ -3878,14 +3875,6 @@ qemuValidateDomainDeviceDefGraphics(const virDomainGraphicsDef *graphics,
}
break;
- case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("vnc graphics are not supported with this QEMU"));
- return -1;
- }
- break;
-
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
if (qemuValidateDomainDeviceDefSPICEGraphics(graphics, driver,
qemuCaps) < 0)
@@ -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;
}
return 0;
--
2.26.2
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.
The default and _LAST case should use virReportEnumRangeError.
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'.
Sorry,
Michal
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.
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
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@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.
© 2016 - 2026 Red Hat, Inc.