[libvirt] [PATCH v2 0/5] Fix up some issues from x and y resolution patches
This is a follow-up series responding to some comments from Jan Tomko. Most importantly, the fact that the errors are not propagated up to the caller, they're only logged. To fix this, the function signatures were changed to return a error status. The patch series was also re-ordered slightly to improve readability (I hope). Jonathon Jongsma (5): qemu: fix documentation for video resolution conf: remove unnecessary NULL checks conf: use glib allocation when parsing video props conf: report errors when parsing video resolution conf: report errors when parsing video acceleration docs/formatdomain.html.in | 12 +++-- src/conf/domain_conf.c | 108 ++++++++++++++++++++++---------------- 2 files changed, 71 insertions(+), 49 deletions(-) -- 2.21.0
The video resolution support that was introduced in 7286279797a34b3083d85bc4556432b5e7ad9fff is specified as a <resolution> sub-element of <model>, not optional attributes of model. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- docs/formatdomain.html.in | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 962766b792..7cc9ada897 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7077,10 +7077,14 @@ qemu-kvm -net nic,model=? /dev/null <code>vgamem</code> (<span class="since">since 1.2.11</span>) to set the size of VGA framebuffer for fallback mode of QXL device. Attribute <code>vram64</code> (<span class="since">since 1.3.3</span>) - extends secondary bar and makes it addressable as 64bit memory. For - resolution settings, there are <code>x</code> and <code>y</code> - (<span class="since">since 5.9.0</span>) optional attributes to set - minimum resolution for model. + extends secondary bar and makes it addressable as 64bit memory. + </p> + <p><span class="since">Since 5.9.0</span>, the <code>model</code> + element may also have an optional <code>resolution</code> sub-element. + The <code>resolution</code> element has attributes <code>x</code> and + <code>y</code> to set the minimum resolution for the video device. This + sub-element is valid for model types "vga", "qxl", "bochs", and + "virtio". </p> </dd> -- 2.21.0
Just above in the function, we return from the function if either x or y are NULL, so there's no need to re-check whether x or y are NULL. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9ee9b44a5e..ad1d3ffea6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15365,20 +15365,16 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node) if (VIR_ALLOC(def) < 0) goto cleanup; - if (x) { - if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("cannot parse video x-resolution '%s'"), x); - goto cleanup; - } + if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot parse video x-resolution '%s'"), x); + goto cleanup; } - if (y) { - if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("cannot parse video y-resolution '%s'"), y); - goto cleanup; - } + if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot parse video y-resolution '%s'"), y); + goto cleanup; } cleanup: -- 2.21.0
In preparation for some other improvements, switch to using glib allocation and g_autofree when parsing the 'acceleration' and 'resolution' properties of the video device. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ad1d3ffea6..269a6ec2c3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15289,7 +15289,7 @@ static virDomainVideoAccelDefPtr virDomainVideoAccelDefParseXML(xmlNodePtr node) { xmlNodePtr cur; - virDomainVideoAccelDefPtr def; + g_autofree virDomainVideoAccelDefPtr def = NULL; int val; g_autofree char *accel2d = NULL; g_autofree char *accel3d = NULL; @@ -15311,8 +15311,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) if (!accel3d && !accel2d && !rendernode) return NULL; - if (VIR_ALLOC(def) < 0) - goto cleanup; + def = g_new0(virDomainVideoAccelDef, 1); if (accel3d) { if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) { @@ -15336,14 +15335,14 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) def->rendernode = virFileSanitizePath(rendernode); cleanup: - return def; + return g_steal_pointer(&def); } static virDomainVideoResolutionDefPtr virDomainVideoResolutionDefParseXML(xmlNodePtr node) { xmlNodePtr cur; - virDomainVideoResolutionDefPtr def; + g_autofree virDomainVideoResolutionDefPtr def = NULL; g_autofree char *x = NULL; g_autofree char *y = NULL; @@ -15362,8 +15361,7 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node) if (!x || !y) return NULL; - if (VIR_ALLOC(def) < 0) - goto cleanup; + def = g_new0(virDomainVideoResolutionDef, 1); if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -15378,7 +15376,7 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node) } cleanup: - return def; + return g_steal_pointer(&def); } static virDomainVideoDriverDefPtr -- 2.21.0
The current code doesn't properly handle errors when parsing a video device's resolution. This patch changes the parse function signature to return an error when we're missing an 'x' or 'y' parameter or when the 'x' or 'y' parameters are not positive integers. No error is returned when no 'resolution' element is found. Previously we were returning a NULL structure for the case where 'x' or 'y' were missing, but were returning an under-specified structure for the other error cases. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 269a6ec2c3..b3f32d0b63 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) return g_steal_pointer(&def); } -static virDomainVideoResolutionDefPtr -virDomainVideoResolutionDefParseXML(xmlNodePtr node) +static int +virDomainVideoResolutionDefParseXML(xmlNodePtr node, + virDomainVideoResolutionDefPtr *res) { xmlNodePtr cur; g_autofree virDomainVideoResolutionDefPtr def = NULL; g_autofree char *x = NULL; g_autofree char *y = NULL; + *res = NULL; cur = node->children; while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (!x && !y && - virXMLNodeNameEqual(cur, "resolution")) { - x = virXMLPropString(cur, "x"); - y = virXMLPropString(cur, "y"); - } + if ((cur->type == XML_ELEMENT_NODE) && + virXMLNodeNameEqual(cur, "resolution")) { + x = virXMLPropString(cur, "x"); + y = virXMLPropString(cur, "y"); + break; } cur = cur->next; } + /* resolution not specified */ + if (cur == NULL) + return 0; + + /* resolution specified, but x or y is missing. report error */ if (!x || !y) - return NULL; + return -1; def = g_new0(virDomainVideoResolutionDef, 1); if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("cannot parse video x-resolution '%s'"), x); - goto cleanup; + return -1; } if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("cannot parse video y-resolution '%s'"), y); - goto cleanup; + return -1; } - cleanup: - return g_steal_pointer(&def); + if (def->x == 0 || def->y == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("resolution values must be greater than 0")); + return -1; + } + + *res = g_steal_pointer(&def); + return 0; } static virDomainVideoDriverDefPtr @@ -15458,7 +15470,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, } def->accel = virDomainVideoAccelDefParseXML(cur); - def->res = virDomainVideoResolutionDefParseXML(cur); + if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("Cannot parse video resolution")); + goto error; + } } if (virXMLNodeNameEqual(cur, "driver")) { if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0) -- 2.21.0
I pushed the first three patches. Comments below On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
The current code doesn't properly handle errors when parsing a video device's resolution.
This patch changes the parse function signature to return an error when we're missing an 'x' or 'y' parameter or when the 'x' or 'y' parameters are not positive integers. No error is returned when no 'resolution' element is found.
Previously we were returning a NULL structure for the case where 'x' or 'y' were missing, but were returning an under-specified structure for the other error cases.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 269a6ec2c3..b3f32d0b63 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) return g_steal_pointer(&def); }
-static virDomainVideoResolutionDefPtr -virDomainVideoResolutionDefParseXML(xmlNodePtr node) +static int +virDomainVideoResolutionDefParseXML(xmlNodePtr node, + virDomainVideoResolutionDefPtr *res) { xmlNodePtr cur; g_autofree virDomainVideoResolutionDefPtr def = NULL; g_autofree char *x = NULL; g_autofree char *y = NULL;
+ *res = NULL; cur = node->children; while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (!x && !y && - virXMLNodeNameEqual(cur, "resolution")) { - x = virXMLPropString(cur, "x"); - y = virXMLPropString(cur, "y"); - } + if ((cur->type == XML_ELEMENT_NODE) && + virXMLNodeNameEqual(cur, "resolution")) { + x = virXMLPropString(cur, "x"); + y = virXMLPropString(cur, "y"); + break; } cur = cur->next; }
This loop can be removed if you move the virXMLNodeNameEqual to the parent function, like how it is handled for parent call of virDomainVirtioOptionsParseXML
+ /* resolution not specified */ + if (cur == NULL) + return 0; + + /* resolution specified, but x or y is missing. report error */ if (!x || !y) - return NULL; + return -1;
def = g_new0(virDomainVideoResolutionDef, 1);
if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("cannot parse video x-resolution '%s'"), x); - goto cleanup; + return -1; }
if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("cannot parse video y-resolution '%s'"), y); - goto cleanup; + return -1; }
- cleanup: - return g_steal_pointer(&def); + if (def->x == 0 || def->y == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("resolution values must be greater than 0")); + return -1; + } + + *res = g_steal_pointer(&def); + return 0; }
This patch is doing two things: converting to the more common error handling pattern, but also adding this error check. Separating them will help review. It's borderline pedantic but this type of error should actually be in the Validate callback machinery instead. Some drivers will fill in a virDomainDef manually in code (like virtualbox). If they accidentally set an x or y value of 0, Validate won't catch it as an error, but the XML parser will catch it later. Generally the XML parser should only throw errors about
static virDomainVideoDriverDefPtr @@ -15458,7 +15470,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, }
def->accel = virDomainVideoAccelDefParseXML(cur); - def->res = virDomainVideoResolutionDefParseXML(cur); + if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("Cannot parse video resolution")); + goto error; + } }
Calling virReporError here will overwrite the error raised by virDomainVideoResolutionDefParseXML. The error reporting machinery doesn't have any smarts to merge errors or anything like that, it needs to be done manually. In this case you can just drop the virReportError entirely I'll be quicker about reviewing v3. Thanks, Cole
On Wed, 2019-11-13 at 13:54 -0500, Cole Robinson wrote:
> I pushed the first three patches. Comments below
>
> On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
> > The current code doesn't properly handle errors when parsing a
> > video
> > device's resolution.
> >
> > This patch changes the parse function signature to return an error
> > when we're missing an 'x' or 'y' parameter or when the 'x' or 'y'
> > parameters are not positive integers. No error is returned when no
> > 'resolution' element is found.
> >
> > Previously we were returning a NULL structure for the case where
> > 'x' or
> > 'y' were missing, but were returning an under-specified structure
> > for
> > the other error cases.
> >
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > ---
> > src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++--------
> > ------
> > 1 file changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 269a6ec2c3..b3f32d0b63 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr
> > node)
> > return g_steal_pointer(&def);
> > }
> >
> > -static virDomainVideoResolutionDefPtr
> > -virDomainVideoResolutionDefParseXML(xmlNodePtr node)
> > +static int
> > +virDomainVideoResolutionDefParseXML(xmlNodePtr node,
> > + virDomainVideoResolutionDefPtr
> > *res)
> > {
> > xmlNodePtr cur;
> > g_autofree virDomainVideoResolutionDefPtr def = NULL;
> > g_autofree char *x = NULL;
> > g_autofree char *y = NULL;
> >
> > + *res = NULL;
> > cur = node->children;
> > while (cur != NULL) {
> > - if (cur->type == XML_ELEMENT_NODE) {
> > - if (!x && !y &&
> > - virXMLNodeNameEqual(cur, "resolution")) {
> > - x = virXMLPropString(cur, "x");
> > - y = virXMLPropString(cur, "y");
> > - }
> > + if ((cur->type == XML_ELEMENT_NODE) &&
> > + virXMLNodeNameEqual(cur, "resolution")) {
> > + x = virXMLPropString(cur, "x");
> > + y = virXMLPropString(cur, "y");
> > + break;
> > }
> > cur = cur->next;
> > }
> >
>
> This loop can be removed if you move the virXMLNodeNameEqual to the
> parent function, like how it is handled for parent call of
> virDomainVirtioOptionsParseXML
>
> > + /* resolution not specified */
> > + if (cur == NULL)
> > + return 0;
> > +
> > + /* resolution specified, but x or y is missing. report error
> > */
> > if (!x || !y)
> > - return NULL;
> > + return -1;
> >
> > def = g_new0(virDomainVideoResolutionDef, 1);
> >
> > if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > _("cannot parse video x-resolution '%s'"),
> > x);
> > - goto cleanup;
> > + return -1;
> > }
> >
> > if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > _("cannot parse video y-resolution '%s'"),
> > y);
> > - goto cleanup;
> > + return -1;
> > }
> >
> > - cleanup:
> > - return g_steal_pointer(&def);
> > + if (def->x == 0 || def->y == 0) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("resolution values must be greater than
> > 0"));
> > + return -1;
> > + }
> > +
> > + *res = g_steal_pointer(&def);
> > + return 0;
> > }
> >
>
> This patch is doing two things: converting to the more common error
> handling pattern, but also adding this error check. Separating them
> will
> help review.
>
> It's borderline pedantic but this type of error should actually be in
> the Validate callback machinery instead. Some drivers will fill in a
> virDomainDef manually in code (like virtualbox). If they accidentally
> set an x or y value of 0, Validate won't catch it as an error, but
> the
> XML parser will catch it later. Generally the XML parser should only
> throw errors about
It seems that this sentence is unfinished?
>
> > static virDomainVideoDriverDefPtr
> > @@ -15458,7 +15470,11 @@
> > virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
> > }
> >
> > def->accel = virDomainVideoAccelDefParseXML(cur);
> > - def->res =
> > virDomainVideoResolutionDefParseXML(cur);
> > + if (virDomainVideoResolutionDefParseXML(cur, &def-
> > >res) < 0) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + "%s", _("Cannot parse video
> > resolution"));
> > + goto error;
> > + }
> > }
>
> Calling virReporError here will overwrite the error raised by
> virDomainVideoResolutionDefParseXML. The error reporting machinery
> doesn't have any smarts to merge errors or anything like that, it
> needs
> to be done manually. In this case you can just drop the
> virReportError
> entirely
>
> I'll be quicker about reviewing v3.
>
> Thanks,
> Cole
On 11/13/19 5:41 PM, Jonathon Jongsma wrote:
On Wed, 2019-11-13 at 13:54 -0500, Cole Robinson wrote:
I pushed the first three patches. Comments below
On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
The current code doesn't properly handle errors when parsing a video device's resolution.
This patch changes the parse function signature to return an error when we're missing an 'x' or 'y' parameter or when the 'x' or 'y' parameters are not positive integers. No error is returned when no 'resolution' element is found.
Previously we were returning a NULL structure for the case where 'x' or 'y' were missing, but were returning an under-specified structure for the other error cases.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++-------- ------ 1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 269a6ec2c3..b3f32d0b63 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) return g_steal_pointer(&def); }
-static virDomainVideoResolutionDefPtr -virDomainVideoResolutionDefParseXML(xmlNodePtr node) +static int +virDomainVideoResolutionDefParseXML(xmlNodePtr node, + virDomainVideoResolutionDefPtr *res) { xmlNodePtr cur; g_autofree virDomainVideoResolutionDefPtr def = NULL; g_autofree char *x = NULL; g_autofree char *y = NULL;
+ *res = NULL; cur = node->children; while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (!x && !y && - virXMLNodeNameEqual(cur, "resolution")) { - x = virXMLPropString(cur, "x"); - y = virXMLPropString(cur, "y"); - } + if ((cur->type == XML_ELEMENT_NODE) && + virXMLNodeNameEqual(cur, "resolution")) { + x = virXMLPropString(cur, "x"); + y = virXMLPropString(cur, "y"); + break; } cur = cur->next; }
This loop can be removed if you move the virXMLNodeNameEqual to the parent function, like how it is handled for parent call of virDomainVirtioOptionsParseXML
+ /* resolution not specified */ + if (cur == NULL) + return 0; + + /* resolution specified, but x or y is missing. report error */ if (!x || !y) - return NULL; + return -1;
def = g_new0(virDomainVideoResolutionDef, 1);
if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("cannot parse video x-resolution '%s'"), x); - goto cleanup; + return -1; }
if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("cannot parse video y-resolution '%s'"), y); - goto cleanup; + return -1; }
- cleanup: - return g_steal_pointer(&def); + if (def->x == 0 || def->y == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("resolution values must be greater than 0")); + return -1; + } + + *res = g_steal_pointer(&def); + return 0; }
This patch is doing two things: converting to the more common error handling pattern, but also adding this error check. Separating them will help review.
It's borderline pedantic but this type of error should actually be in the Validate callback machinery instead. Some drivers will fill in a virDomainDef manually in code (like virtualbox). If they accidentally set an x or y value of 0, Validate won't catch it as an error, but the XML parser will catch it later. Generally the XML parser should only throw errors about
It seems that this sentence is unfinished?
I meant to delete that unformed thought because it was going to turn into more than a sentence and would distract from this patch :) Generally the XML parser should only raise errors that are specific to the XML input. Anything that's validating a value after it has already been parsed is better served in the Validate callbacks. The existing code is not the best guide for this though, it's just the goal we should shoot for IMO - Cole
In order to differentiate between a configuration that does not specify any video acceleration and one that is incorrectly specified, change the signature of virDomainVideoAccelDefParseXML() to return an error response. If any of the values are invalid, report an error rather than returning a partially-specified accel object. If no 'acceleration' node is found, do not report an error since it is optional. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b3f32d0b63..1c3fc5c4ce 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15285,8 +15285,8 @@ virDomainVideoDefaultType(const virDomainDef *def) } } -static virDomainVideoAccelDefPtr -virDomainVideoAccelDefParseXML(xmlNodePtr node) +static int +virDomainVideoAccelDefParseXML(xmlNodePtr node, virDomainVideoAccelDefPtr *accel) { xmlNodePtr cur; g_autofree virDomainVideoAccelDefPtr def = NULL; @@ -15295,21 +15295,25 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) g_autofree char *accel3d = NULL; g_autofree char *rendernode = NULL; + *accel = NULL; cur = node->children; while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (!accel3d && !accel2d && - virXMLNodeNameEqual(cur, "acceleration")) { - accel3d = virXMLPropString(cur, "accel3d"); - accel2d = virXMLPropString(cur, "accel2d"); - rendernode = virXMLPropString(cur, "rendernode"); - } + if ((cur->type == XML_ELEMENT_NODE) && + virXMLNodeNameEqual(cur, "acceleration")) { + accel3d = virXMLPropString(cur, "accel3d"); + accel2d = virXMLPropString(cur, "accel2d"); + rendernode = virXMLPropString(cur, "rendernode"); + break; } cur = cur->next; } + /* no acceleration node was specified */ + if (cur == NULL) + return 0; + if (!accel3d && !accel2d && !rendernode) - return NULL; + return -1; def = g_new0(virDomainVideoAccelDef, 1); @@ -15317,7 +15321,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown accel3d value '%s'"), accel3d); - goto cleanup; + return -1; } def->accel3d = val; } @@ -15326,7 +15330,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown accel2d value '%s'"), accel2d); - goto cleanup; + return -1; } def->accel2d = val; } @@ -15334,8 +15338,8 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) if (rendernode) def->rendernode = virFileSanitizePath(rendernode); - cleanup: - return g_steal_pointer(&def); + *accel = g_steal_pointer(&def); + return 0; } static int @@ -15469,7 +15473,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(primary); } - def->accel = virDomainVideoAccelDefParseXML(cur); + if (virDomainVideoAccelDefParseXML(cur, &def->accel) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("Cannot parse video acceleration")); + goto error; + } if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Cannot parse video resolution")); -- 2.21.0
On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
In order to differentiate between a configuration that does not specify any video acceleration and one that is incorrectly specified, change the signature of virDomainVideoAccelDefParseXML() to return an error response.
If any of the values are invalid, report an error rather than returning a partially-specified accel object. If no 'acceleration' node is found, do not report an error since it is optional.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b3f32d0b63..1c3fc5c4ce 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15285,8 +15285,8 @@ virDomainVideoDefaultType(const virDomainDef *def) } }
-static virDomainVideoAccelDefPtr -virDomainVideoAccelDefParseXML(xmlNodePtr node) +static int +virDomainVideoAccelDefParseXML(xmlNodePtr node, virDomainVideoAccelDefPtr *accel) { xmlNodePtr cur; g_autofree virDomainVideoAccelDefPtr def = NULL; @@ -15295,21 +15295,25 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) g_autofree char *accel3d = NULL; g_autofree char *rendernode = NULL;
+ *accel = NULL; cur = node->children; while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (!accel3d && !accel2d && - virXMLNodeNameEqual(cur, "acceleration")) { - accel3d = virXMLPropString(cur, "accel3d"); - accel2d = virXMLPropString(cur, "accel2d"); - rendernode = virXMLPropString(cur, "rendernode"); - } + if ((cur->type == XML_ELEMENT_NODE) && + virXMLNodeNameEqual(cur, "acceleration")) { + accel3d = virXMLPropString(cur, "accel3d"); + accel2d = virXMLPropString(cur, "accel2d"); + rendernode = virXMLPropString(cur, "rendernode"); + break; } cur = cur->next; }
THe virXMLNodeNameEqual check should be moved to the parent, same as mentioned in patch 4
+ /* no acceleration node was specified */ + if (cur == NULL) + return 0; + if (!accel3d && !accel2d && !rendernode) - return NULL; + return -1;
def = g_new0(virDomainVideoAccelDef, 1);
@@ -15317,7 +15321,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown accel3d value '%s'"), accel3d); - goto cleanup; + return -1; } def->accel3d = val; } @@ -15326,7 +15330,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown accel2d value '%s'"), accel2d); - goto cleanup; + return -1; } def->accel2d = val; } @@ -15334,8 +15338,8 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) if (rendernode) def->rendernode = virFileSanitizePath(rendernode);
- cleanup: - return g_steal_pointer(&def); + *accel = g_steal_pointer(&def); + return 0; }
static int @@ -15469,7 +15473,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(primary); }
- def->accel = virDomainVideoAccelDefParseXML(cur); + if (virDomainVideoAccelDefParseXML(cur, &def->accel) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("Cannot parse video acceleration")); + goto error; + } if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Cannot parse video resolution"));
Same error overwriting issue here as in patch 4 - Cole
participants (2)
-
Cole Robinson -
Jonathon Jongsma