4:40 p.m.
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
4:40 p.m.
New subject: [libvirt] [PATCH 1/5] qemu: fix documentation for video resolution
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(a)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
5:23 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: fix documentation for video resolution
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(a)redhat.com>
---
docs/formatdomain.html.in | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:40 p.m.
New subject: [libvirt] [PATCH 2/5] conf: Return error when resolution values are invalid
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(a)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
4:40 p.m.
New subject: [libvirt] [PATCH 3/5] conf: remove unnecessary NULL checks
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(a)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
4:40 p.m.
New subject: [libvirt] [PATCH 4/5] conf: ensure both resolution values are non-zero
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(a)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
5:24 a.m.
New subject: [libvirt] [PATCH 4/5] conf: ensure both resolution values are non-zero
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(a)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
11:11 a.m.
New subject: [libvirt] [PATCH 4/5] conf: ensure both resolution values are non-zero
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(a)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
4:40 p.m.
New subject: [libvirt] [PATCH 5/5] conf: report errors when parsing video accel
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(a)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
5:28 a.m.
New subject: [libvirt] [PATCH 5/5] conf: report errors when parsing video accel
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(a)redhat.com>
---
src/conf/domain_conf.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
1860
days inactive
1865
days old
9 comments
2 participants
participants (2)
-
Jonathon Jongsma -
Ján Tomko