2:35 a.m.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3ab7cd2666..2a94566047 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5840,14 +5840,14 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
host->socket = virXMLPropString(hostnode, "socket");
if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX &&
- host->socket == NULL) {
+ virDomainStorageNetHostSocketValidate(host)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing socket for unix transport"));
goto cleanup;
}
if (host->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX &&
- host->socket != NULL) {
+ !virDomainStorageNetHostSocketValidate(host)) {
virReportError(VIR_ERR_XML_ERROR,
_("transport '%1$s' does not support socket
attribute"),
transport);
@@ -11024,30 +11024,8 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef
*def,
VIR_XML_PROP_REQUIRED, &def->type) < 0)
goto error;
- 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 '%1$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 '%1$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 (virDomainGraphicsListenDefValidate(def, graphics, graphicsType))
+ goto error;
if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
if (address && addressCompat && STRNEQ(address, addressCompat))
{
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 9265fef4f9..b8b8f941cc 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2669,6 +2669,38 @@ virDomainGraphicsDefValidate(const virDomainDef *def,
return 0;
}
+int
+virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
+ const virDomainGraphicsDef *graphics,
+ const char *graphicsType)
+{
+ 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 '%1$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 '%1$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;
+}
+
static int
virDomainIOMMUDefValidate(const virDomainIOMMUDef *iommu)
{
@@ -2898,3 +2930,13 @@ virDomainDeviceDefValidate(const virDomainDeviceDef *dev,
return 0;
}
+
+int
+virDomainStorageNetHostSocketValidate(const virStorageNetHostDef *host)
+
+{
+ if (host->socket)
+ return 0;
+ else
+ return -1;
+}
diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h
index fc441cef5b..58ee9b7da5 100644
--- a/src/conf/domain_validate.h
+++ b/src/conf/domain_validate.h
@@ -47,3 +47,10 @@ int virDomainDiskDefSourceLUNValidate(const virStorageSource *src);
int virDomainDefOSValidate(const virDomainDef *def,
virDomainXMLOption *xmlopt);
+
+int virDomainStorageNetHostSocketValidate(const virStorageNetHostDef *host);
+
+int
+virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
+ const virDomainGraphicsDef *graphics,
+ const char *graphicsType);
--
2.40.0
4:34 a.m.
In the summary line please shortly describe the change itself e.g.:
conf: Move validation of graphics transport out of parser
Any further description can be in the body, including mentioning issues
and other less-relevant information.
Additionally in my reply to your application where I've pointed you to
our guidance of how to send patches I've pointed you to:
https://www.libvirt.org/hacking.html#submitting-patches
The very next paragraph (direct anchor link:
https://www.libvirt.org/hacking.html#developer-certificate-of-origin )
states:
Developer Certificate of Origin
===============================
Contributors to libvirt projects **must** assert that they are
in compliance with the `Developer Certificate of Origin
1.1 <https://developercertificate.org/>`__. This is achieved by
adding a "Signed-off-by" line containing the contributor's name
and e-mail to every commit message. The presence of this line
attests that the contributor has read the above lined DCO and
agrees with its statements.
Make sure to follow that guidance too. Also note that pseudonyms are not
accepted.
On Mon, Apr 03, 2023 at 13:05:53 +0530, skran wrote:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3ab7cd2666..2a94566047 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5840,14 +5840,14 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
host->socket = virXMLPropString(hostnode, "socket");
if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX &&
- host->socket == NULL) {
+ virDomainStorageNetHostSocketValidate(host)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing socket for unix transport"));
goto cleanup;
}
if (host->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX &&
- host->socket != NULL) {
+ !virDomainStorageNetHostSocketValidate(host)) {
virReportError(VIR_ERR_XML_ERROR,
_("transport '%1$s' does not support socket
attribute"),
transport);
Both hunks above don't really move the validation anywhere. It just
moves one condition to different file which in fact decreases
readability of the code.
Additionally virDomainStorageNetHostSocketValidate returns 0 or -1 but
you don't check it explicitly, which further obscures the code.
This part of the patch doesn't make much sense.
@@ -11024,30 +11024,8 @@
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
VIR_XML_PROP_REQUIRED, &def->type) < 0)
goto error;
- 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 '%1$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 '%1$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 (virDomainGraphicsListenDefValidate(def, graphics, graphicsType))
+ goto error;
While this bit technically doesn't really move the validation out of the
parser [1] this is an acceptable change.
[1] moving the validation out of the parser means that the error is not
raised from the parser altogether.
if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
if (address && addressCompat && STRNEQ(address, addressCompat))
{
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 9265fef4f9..b8b8f941cc 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2669,6 +2669,38 @@ virDomainGraphicsDefValidate(const virDomainDef *def,
return 0;
}
+int
+virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
+ const virDomainGraphicsDef *graphics,
+ const char *graphicsType)
The only caller passes graphicsType via:
const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
Since it's the only use of 'graphicsType' in the caller move that bit
here and remove the argument.
+{
+ 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 '%1$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 '%1$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;
+}
5:17 a.m.
On 4/3/23 11:34, Peter Krempa wrote:
In the summary line please shortly describe the change itself e.g.:
conf: Move validation of graphics transport out of parser
Any further description can be in the body, including mentioning issues
and other less-relevant information.
Additionally in my reply to your application where I've pointed you to
our guidance of how to send patches I've pointed you to:
https://www.libvirt.org/hacking.html#submitting-patches
The very next paragraph (direct anchor link:
https://www.libvirt.org/hacking.html#developer-certificate-of-origin )
states:
Developer Certificate of Origin
===============================
Contributors to libvirt projects **must** assert that they are
in compliance with the `Developer Certificate of Origin
1.1 <https://developercertificate.org/>`__. This is achieved by
adding a "Signed-off-by" line containing the contributor's name
and e-mail to every commit message. The presence of this line
attests that the contributor has read the above lined DCO and
agrees with its statements.
Make sure to follow that guidance too. Also note that pseudonyms are not
accepted.
On Mon, Apr 03, 2023 at 13:05:53 +0530, skran wrote:
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3ab7cd2666..2a94566047 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5840,14 +5840,14 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
> host->socket = virXMLPropString(hostnode, "socket");
>
> if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX &&
> - host->socket == NULL) {
> + virDomainStorageNetHostSocketValidate(host)) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("missing socket for unix transport"));
> goto cleanup;
> }
>
> if (host->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX &&
> - host->socket != NULL) {
> + !virDomainStorageNetHostSocketValidate(host)) {
> virReportError(VIR_ERR_XML_ERROR,
> _("transport '%1$s' does not support socket
attribute"),
> transport);
Both hunks above don't really move the validation anywhere. It just
moves one condition to different file which in fact decreases
readability of the code.
Just for the record: I highly suggest you take a look how XML parsing is
done. The virDomainDefParseNode() looks like a good start.
Long story short, XML parsing is done in three steps:
1) the actual parsing, when virDomainDef (or any other definition struct
for other objects, like virNetwork, virNodeDev, etc.) is filled with
data from given XML document,
2) PostParse - when missing information is filled in (e.g. MAC addresses
are filled to vNICs), and finally
3) Validate - when the whole definition struct is checked for errors
(e.g. there are no devices with duplicitous alias).
Now, we have a lot of old code which combined these three steps into one
and we are working slowly to get rid of it, but hopefully now you see
why this hunk in particual isn't step in the right direction.
Michal
598
days inactive
598
days old
2 comments
3 participants
participants (3)
-
Michal Prívozník -
Peter Krempa -
skran