[libvirt] [PATCH 0/5] Fix up some issues from x and y resolution patches
I already had some of these changes locally from my review, so I decided to send a quick cleanup series on the x and y resolution changes myself. The last patch in the series is a similar cleanup to a related function that was probably the inspiration for the resolution parsing function. It changes the behavior of this function slightly, but I think the previous behavior should be considered a bug. An illustration of the change in behavior: previously, the following configuration (note: invalid value for accel2d): <acceleration accel2d='foo' accel3d='yes' rendernode='/dev/dri/test'/> would have resulted in the the parse function returning the following struct: { accel2d = 0; /* default value */ accel3d = 1; /* YES */ rendernode = NULL /* default value - not parsed due to accel2d error */ } After this patch, the parse function returns NULL instead. I think that's an improvement, but it is different than the previous behavior. Jonathon Jongsma (5): qemu: fix documentation for video resolution conf: Return error when resolution values are invalid conf: remove unnecessary NULL checks conf: ensure both resolution values are non-zero conf: report errors when parsing video accel docs/formatdomain.html.in | 12 ++++++---- src/conf/domain_conf.c | 46 +++++++++++++++++++-------------------- 2 files changed, 30 insertions(+), 28 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
On Fri, Oct 18, 2019 at 04:40:19PM -0500, Jonathon Jongsma wrote:
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(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Report an error and return NULL when either the 'x' or 'y' resolution values cannot be converted to unsigned integers rather than returning the incomplete 'def' variable. Switch 'def' to an autofree variable to simplify the logic and remove the goto. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 88e93f6fb8..5657faf039 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15349,7 +15349,7 @@ static virDomainVideoResolutionDefPtr virDomainVideoResolutionDefParseXML(xmlNodePtr node) { xmlNodePtr cur; - virDomainVideoResolutionDefPtr def; + g_autofree virDomainVideoResolutionDefPtr def = NULL; g_autofree char *x = NULL; g_autofree char *y = NULL; @@ -15368,14 +15368,13 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node) if (!x || !y) return NULL; - if (VIR_ALLOC(def) < 0) - goto cleanup; + def = g_new0(virDomainVideoResolutionDef, 1); 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; + return NULL; } } @@ -15383,12 +15382,11 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node) if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("cannot parse video y-resolution '%s'"), y); - goto cleanup; + return NULL; } } - cleanup: - return def; + return g_steal_pointer(&def); } static virDomainVideoDriverDefPtr -- 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 5657faf039..a446ce4d62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15370,20 +15370,16 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node) def = g_new0(virDomainVideoResolutionDef, 1); - if (x) { - if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("cannot parse video x-resolution '%s'"), x); - return NULL; - } + if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot parse video x-resolution '%s'"), x); + return NULL; } - if (y) { - if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("cannot parse video y-resolution '%s'"), y); - return NULL; - } + if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot parse video y-resolution '%s'"), y); + return NULL; } return g_steal_pointer(&def); -- 2.21.0
Since the users of the resolution expect the x and y values to be non-zero, enforce it in the parser and report an error if either is zero. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a446ce4d62..38f8b37b69 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15382,6 +15382,12 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node) return NULL; } + if (def->x == 0 || def->y == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("resolution values must be greater than 0")); + return NULL; + } + return g_steal_pointer(&def); } -- 2.21.0
On Fri, Oct 18, 2019 at 04:40:22PM -0500, Jonathon Jongsma wrote:
Since the users of the resolution expect the x and y values to be non-zero, enforce it in the parser and report an error if either is zero.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 6 ++++++ 1 file changed, 6 insertions(+)
This only works when somebody explicitly specifies x='0', but does not catch the case where x is omitted from the XML completely Jano
On Wed, 2019-10-23 at 12:24 +0200, Ján Tomko wrote:
On Fri, Oct 18, 2019 at 04:40:22PM -0500, Jonathon Jongsma wrote:
Since the users of the resolution expect the x and y values to be non-zero, enforce it in the parser and report an error if either is zero.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 6 ++++++ 1 file changed, 6 insertions(+)
This only works when somebody explicitly specifies x='0', but does not catch the case where x is omitted from the XML completely
That's true with respect to the changes within this diff, but earlier in the function we already ensure that 'x' and 'y' are non-null. So if the attributes were omitted, we would have already failed before this point. Jonathon
If any of the values are invalid, report an error and return NULL rather than returning a partially-specified accel object. Convert to g_autofree as well to simplify logic and remove the goto. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 38f8b37b69..aa8a38a849 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15295,7 +15295,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; @@ -15317,14 +15317,13 @@ 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) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown accel3d value '%s'"), accel3d); - goto cleanup; + return NULL; } def->accel3d = val; } @@ -15333,7 +15332,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown accel2d value '%s'"), accel2d); - goto cleanup; + return NULL; } def->accel2d = val; } @@ -15341,8 +15340,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) if (rendernode) def->rendernode = virFileSanitizePath(rendernode); - cleanup: - return def; + return g_steal_pointer(&def); } static virDomainVideoResolutionDefPtr -- 2.21.0
On Fri, Oct 18, 2019 at 04:40:23PM -0500, Jonathon Jongsma wrote:
If any of the values are invalid, report an error and return NULL rather than returning a partially-specified accel object. Convert to g_autofree
This is marginally better, but the reported errors will only get logged, not reported - since the parent function does not treat a NULL accel as a failure. To properly propagate the errors to the user, the function needs to distinguish between failure and no accel specified. (Same for virDomainVideoResolutionDefParseXML which I guess was modelled after this one) Jano
as well to simplify logic and remove the goto.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/domain_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
participants (2)
-
Jonathon Jongsma -
Ján Tomko