12:56 p.m.
- Based on https://gitlab.com/libvirt/libvirt/-/issues/7 specific
recommendation about moving validation checks into *PostParse.
- syntax-check and tests passing verified.
- I Considered creating a new "Validate" function, but according to the issue
I used the PostParse Function.
Nicolas Brignone (1):
conf: move graphics validation checks out of *ParseXML function.
src/conf/domain_conf.c | 66 +++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 30 deletions(-)
--
2.25.2
12:56 p.m.
New subject: [libvirt PATCH 1/1] conf: move graphics validation checks out of *ParseXML function.
Existing virDomainDefPostParseGraphics function seems to be the right
place to put this validations.
After moving this validation, one less argument is needed in
virDomainGraphicsListenDefParseXML, so removing the "graphics" argument
from the function signature.
Signed-off-by: Nicolas Brignone <nmbrignone(a)gmail.com>
---
src/conf/domain_conf.c | 66 +++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 30 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8d328819..3228f12a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4804,13 +4804,15 @@ virDomainDefPostParseTimer(virDomainDefPtr def)
}
-static void
+static int
virDomainDefPostParseGraphics(virDomainDef *def)
{
size_t i;
for (i = 0; i < def->ngraphics; i++) {
+ size_t j;
virDomainGraphicsDefPtr graphics = def->graphics[i];
+ const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
/* If spice graphics is configured without ports and with autoport='no'
* then we start qemu with Spice to not listen anywhere. Let's convert
@@ -4826,8 +4828,38 @@ virDomainDefPostParseGraphics(virDomainDef *def)
VIR_FREE(glisten->address);
glisten->type = VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE;
}
+
+ }
+
+ for (j = 0; j < graphics->nListens; j++) {
+ virDomainGraphicsListenDefPtr glisten = virDomainGraphicsGetListen(graphics,
j);
+ switch (glisten->type) {
+ case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
+ if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+ graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("listen type 'socket' is not available for
"
+ "graphics type '%s'"),
graphicsType);
+ return -1;
+ }
+ break;
+ case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
+ if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+ graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("listen type 'none' is not available for
"
+ "graphics type '%s'"),
graphicsType);
+ return -1;
+ }
+ break;
+ case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
+ case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
+ case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
+ break;
+ }
}
}
+ return 0;
}
@@ -5915,7 +5947,8 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
/* clean up possibly duplicated metadata entries */
virXMLNodeSanitizeNamespaces(def->metadata);
- virDomainDefPostParseGraphics(def);
+ if (virDomainDefPostParseGraphics(def) < 0)
+ return -1;
if (virDomainDefPostParseCPU(def) < 0)
return -1;
@@ -14140,13 +14173,11 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
*/
static int
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
- virDomainGraphicsDefPtr graphics,
xmlNodePtr node,
xmlNodePtr parent,
unsigned int flags)
{
int ret = -1;
- const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
int tmp, typeVal;
g_autofree char *type = virXMLPropString(node, "type");
g_autofree char *address = virXMLPropString(node, "address");
@@ -14175,31 +14206,6 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr
def,
}
def->type = typeVal;
- switch (def->type) {
- case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
- if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
- graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("listen type 'socket' is not available for
"
- "graphics type '%s'"), graphicsType);
- goto error;
- }
- break;
- case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
- if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
- graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("listen type 'none' is not available for
"
- "graphics type '%s'"), graphicsType);
- goto error;
- }
- break;
- case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
- case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
- case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
- break;
- }
-
if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
if (address && addressCompat && STRNEQ(address, addressCompat))
{
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -14309,7 +14315,7 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDefPtr def,
goto cleanup;
for (i = 0; i < nListens; i++) {
- if (virDomainGraphicsListenDefParseXML(&def->listens[i], def,
+ if (virDomainGraphicsListenDefParseXML(&def->listens[i],
listenNodes[i],
i == 0 ? node : NULL,
flags) < 0)
--
2.25.2
3:08 p.m.
New subject: [libvirt PATCH 1/1] conf: move graphics validation checks out of *ParseXML function.
On Wed, 2020-07-22 at 14:56 -0300, Nicolas Brignone wrote:
Existing virDomainDefPostParseGraphics function seems to be the
right
place to put this validations.
After moving this validation, one less argument is needed in
virDomainGraphicsListenDefParseXML, so removing the "graphics"
argument
from the function signature.
Signed-off-by: Nicolas Brignone <nmbrignone(a)gmail.com>
---
src/conf/domain_conf.c | 66 +++++++++++++++++++++++-----------------
--
1 file changed, 36 insertions(+), 30 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8d328819..3228f12a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4804,13 +4804,15 @@ virDomainDefPostParseTimer(virDomainDefPtr
def)
}
-static void
+static int
virDomainDefPostParseGraphics(virDomainDef *def)
{
size_t i;
for (i = 0; i < def->ngraphics; i++) {
+ size_t j;
virDomainGraphicsDefPtr graphics = def->graphics[i];
+ const char *graphicsType =
virDomainGraphicsTypeToString(graphics->type);
/* If spice graphics is configured without ports and with
autoport='no'
* then we start qemu with Spice to not listen
anywhere. Let's convert
@@ -4826,8 +4828,38 @@ virDomainDefPostParseGraphics(virDomainDef
*def)
VIR_FREE(glisten->address);
glisten->type =
VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE;
}
+
+ }
+
+ for (j = 0; j < graphics->nListens; j++) {
+ virDomainGraphicsListenDefPtr glisten =
virDomainGraphicsGetListen(graphics, j);
+ switch (glisten->type) {
+ case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
+ if (graphics->type !=
VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+ graphics->type !=
VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("listen type 'socket' is not
available for "
+ "graphics type '%s'"),
graphicsType);
+ return -1;
+ }
+ break;
+ case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
+ if (graphics->type !=
VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+ graphics->type !=
VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("listen type 'none' is not
available for "
+ "graphics type '%s'"),
graphicsType);
+ return -1;
+ }
+ break;
+ case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
+ case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
+ case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
+ break;
+ }
}
}
+ return 0;
}
@@ -5915,7 +5947,8 @@ virDomainDefPostParseCommon(virDomainDefPtr
def,
/* clean up possibly duplicated metadata entries */
virXMLNodeSanitizeNamespaces(def->metadata);
- virDomainDefPostParseGraphics(def);
+ if (virDomainDefPostParseGraphics(def) < 0)
+ return -1;
if (virDomainDefPostParseCPU(def) < 0)
return -1;
@@ -14140,13 +14173,11 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr
node,
*/
static int
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr
def,
- virDomainGraphicsDefPtr graphics,
xmlNodePtr node,
xmlNodePtr parent,
unsigned int flags)
{
int ret = -1;
- const char *graphicsType =
virDomainGraphicsTypeToString(graphics->type);
int tmp, typeVal;
g_autofree char *type = virXMLPropString(node, "type");
g_autofree char *address = virXMLPropString(node, "address");
@@ -14175,31 +14206,6 @@
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
}
def->type = typeVal;
- switch (def->type) {
- case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
- if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
- graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("listen type 'socket' is not available
for "
- "graphics type '%s'"), graphicsType);
- goto error;
- }
- break;
- case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
- if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
- graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("listen type 'none' is not available
for "
- "graphics type '%s'"), graphicsType);
- goto error;
- }
- break;
- case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
- case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
- case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
- break;
- }
-
if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
if (address && addressCompat && STRNEQ(address,
addressCompat)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -14309,7 +14315,7 @@
virDomainGraphicsListensParseXML(virDomainGraphicsDefPtr def,
goto cleanup;
for (i = 0; i < nListens; i++) {
- if (virDomainGraphicsListenDefParseXML(&def->listens[i],
def,
+ if (virDomainGraphicsListenDefParseXML(&def->listens[i],
listenNodes[i],
i == 0 ? node :
NULL,
flags) < 0)
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
It looks like there are still some additional validation checks in this
function that could be also moved in subsequent patches if you're
motivated.
11:41 a.m.
New subject: [libvirt PATCH 1/1] conf: move graphics validation checks out of *ParseXML function.
Hi!
Is this going to be merged? Do I need to do something else?
Thanks!
On Fri, Jul 24, 2020 at 5:08 PM Jonathon Jongsma <jjongsma(a)redhat.com>
wrote:
On Wed, 2020-07-22 at 14:56 -0300, Nicolas Brignone wrote:
> Existing virDomainDefPostParseGraphics function seems to be the right
> place to put this validations.
>
> After moving this validation, one less argument is needed in
> virDomainGraphicsListenDefParseXML, so removing the "graphics"
> argument
> from the function signature.
>
> Signed-off-by: Nicolas Brignone <nmbrignone(a)gmail.com>
> ---
> src/conf/domain_conf.c | 66 +++++++++++++++++++++++-----------------
> --
> 1 file changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8d328819..3228f12a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4804,13 +4804,15 @@ virDomainDefPostParseTimer(virDomainDefPtr
> def)
> }
>
>
> -static void
> +static int
> virDomainDefPostParseGraphics(virDomainDef *def)
> {
> size_t i;
>
> for (i = 0; i < def->ngraphics; i++) {
> + size_t j;
> virDomainGraphicsDefPtr graphics = def->graphics[i];
> + const char *graphicsType =
> virDomainGraphicsTypeToString(graphics->type);
>
> /* If spice graphics is configured without ports and with
> autoport='no'
> * then we start qemu with Spice to not listen
> anywhere. Let's convert
> @@ -4826,8 +4828,38 @@ virDomainDefPostParseGraphics(virDomainDef
> *def)
> VIR_FREE(glisten->address);
> glisten->type =
> VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE;
> }
> +
> + }
> +
> + for (j = 0; j < graphics->nListens; j++) {
> + virDomainGraphicsListenDefPtr glisten =
> virDomainGraphicsGetListen(graphics, j);
> + switch (glisten->type) {
> + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
> + if (graphics->type !=
> VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> + graphics->type !=
> VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("listen type 'socket' is not
> available for "
> + "graphics type '%s'"),
> graphicsType);
> + return -1;
> + }
> + break;
> + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> + if (graphics->type !=
> VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
> + graphics->type !=
> VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("listen type 'none' is not
> available for "
> + "graphics type '%s'"),
> graphicsType);
> + return -1;
> + }
> + break;
> + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
> + break;
> + }
> }
> }
> + return 0;
> }
>
>
> @@ -5915,7 +5947,8 @@ virDomainDefPostParseCommon(virDomainDefPtr
> def,
> /* clean up possibly duplicated metadata entries */
> virXMLNodeSanitizeNamespaces(def->metadata);
>
> - virDomainDefPostParseGraphics(def);
> + if (virDomainDefPostParseGraphics(def) < 0)
> + return -1;
>
> if (virDomainDefPostParseCPU(def) < 0)
> return -1;
> @@ -14140,13 +14173,11 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr
> node,
> */
> static int
> virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr
> def,
> - virDomainGraphicsDefPtr graphics,
> xmlNodePtr node,
> xmlNodePtr parent,
> unsigned int flags)
> {
> int ret = -1;
> - const char *graphicsType =
> virDomainGraphicsTypeToString(graphics->type);
> int tmp, typeVal;
> g_autofree char *type = virXMLPropString(node, "type");
> g_autofree char *address = virXMLPropString(node, "address");
> @@ -14175,31 +14206,6 @@
> virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
> }
> def->type = typeVal;
>
> - switch (def->type) {
> - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
> - if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> - graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("listen type 'socket' is not available
> for "
> - "graphics type '%s'"),
graphicsType);
> - goto error;
> - }
> - break;
> - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> - if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
> - graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("listen type 'none' is not available
> for "
> - "graphics type '%s'"),
graphicsType);
> - goto error;
> - }
> - break;
> - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
> - break;
> - }
> -
> if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
> if (address && addressCompat && STRNEQ(address,
> addressCompat)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -14309,7 +14315,7 @@
> virDomainGraphicsListensParseXML(virDomainGraphicsDefPtr def,
> goto cleanup;
>
> for (i = 0; i < nListens; i++) {
> - if (virDomainGraphicsListenDefParseXML(&def->listens[i],
> def,
> + if (virDomainGraphicsListenDefParseXML(&def->listens[i],
> listenNodes[i],
> i == 0 ? node :
> NULL,
> flags) < 0)
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
It looks like there are still some additional validation checks in this
function that could be also moved in subsequent patches if you're
motivated.
2:11 p.m.
New subject: [libvirt PATCH 1/1] conf: move graphics validation checks out of *ParseXML function.
On 7/30/20 6:41 PM, Nicolás Brignone wrote:
Hi!
Is this going to be merged? Do I need to do something else?
Thanks!
Hi, unfortunately it missed the feature freeze for the upcoming release.
We are expecting the release to happen by start of the next week. I will
merge it afterwards.
Michal
1576
days inactive
1584
days old
4 comments
4 participants
participants (4)
-
Jonathon Jongsma -
Michal Privoznik -
Nicolas Brignone -
Nicolás Brignone