From: skran <shiva_kr@riseup.net>

In an effort to separate the validation steps from the Parse stage, a
part of the validation of virDomainGraphicsListenDef has been moved to
domain_validate.h.

Signed-off-by: K Shiva <shiva_kr@riseup.net>
---

This is a v2 of:
https://listman.redhat.com/archives/libvir-list/2023-April/239205.html

diff to v1:
- Made virDomainGraphicsListenDefValidate() static, called by
  virDomainGraphicsDefListensValidate()
- Removed unused Parameter (*graphics) out of Parse Function definition i.e
  virDomainGraphicsListenParseXML() 
- Corrected code format

 src/conf/domain_conf.c     |  9 ++---
 src/conf/domain_validate.c | 68 ++++++++++++++++++++------------------
 src/conf/domain_validate.h |  4 ---
 3 files changed, 37 insertions(+), 44 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1e0ac737bb..746bb4efdf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5836,7 +5836,7 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
 
     host->socket = virXMLPropString(hostnode, "socket");
 
-    // Socket Validation
+    /* Socket Validation */
     if (virDomainStorageNetHostSocketValidate(host, transport) < 0)
         goto cleanup;
 
@@ -10975,7 +10975,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
 /**
  * virDomainGraphicsListenDefParseXML:
  * @def: listen def pointer to be filled
- * @graphics: graphics def pointer
  * @node: xml node of <listen/> element
  * @parent: xml node of <graphics/> element
  * @flags: bit-wise or of VIR_DOMAIN_DEF_PARSE_*
@@ -10987,7 +10986,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
  */
 static int
 virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
-                                   virDomainGraphicsDef *graphics,
                                    xmlNodePtr node,
                                    xmlNodePtr parent,
                                    unsigned int flags)
@@ -11009,9 +11007,6 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
                        VIR_XML_PROP_REQUIRED, &def->type) < 0)
         goto error;
 
-    if (virDomainGraphicsListenDefValidate(def, graphics) == -1)
-        goto error;
-
     if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
         if (address && addressCompat && STRNEQ(address, addressCompat)) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -11114,7 +11109,7 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDef *def,
         def->listens = g_new0(virDomainGraphicsListenDef, nListens);
 
         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)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 10f8242c84..a84dbe8bc9 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2627,6 +2627,39 @@ virDomainAudioDefValidate(const virDomainDef *def,
     return 0;
 }
 
+static int
+virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
+                                   const virDomainGraphicsDef *graphics)
+{
+    const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
+
+    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
 virDomainGraphicsDefListensValidate(const virDomainGraphicsDef *def)
 {
@@ -2640,6 +2673,8 @@ virDomainGraphicsDefListensValidate(const virDomainGraphicsDef *def)
                              "listen type 'network'"));
             return -1;
         }
+        if (virDomainGraphicsListenDefValidate(&def->listens[i], def) < 0)
+            return -1;
     }
 
     return 0;
@@ -2669,39 +2704,6 @@ virDomainGraphicsDefValidate(const virDomainDef *def,
     return 0;
 }
 
-int
-virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
-                                   const virDomainGraphicsDef *graphics)
-{
-    const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
-
-    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)
 {
diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h
index baeae4b2a3..7772a62e18 100644
--- a/src/conf/domain_validate.h
+++ b/src/conf/domain_validate.h
@@ -48,9 +48,5 @@ int virDomainDiskDefSourceLUNValidate(const virStorageSource *src);
 int virDomainDefOSValidate(const virDomainDef *def,
                            virDomainXMLOption *xmlopt);
 
-int
-virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
-                                   const virDomainGraphicsDef *graphics);
-
 int virDomainStorageNetHostSocketValidate(const virStorageNetHostDef *host,
                                     const char *transport);
-- 
2.40.0

On 4/5/23 19:28, K Shiva wrote:
> From: skran <shiva_kr@riseup.net>
> 
> In an effort to separate the validation steps from the Parse stage, a
> part of the validation of virDomainGraphicsListenDef has been moved to
> domain_validate.h.
> 
> Signed-off-by: K Shiva <shiva_kr@riseup.net>

Now, this is perfect! This message tells you what the commit does
(except, the patch does a bit more than what the commit message
describes, but more on that below).

> ---
> 
> This is a v2 of:
> https://listman.redhat.com/archives/libvir-list/2023-April/239205.html
> 
> diff to v1:
> - Made virDomainGraphicsListenDefValidate() static, called by
>   virDomainGraphicsDefListensValidate()
> - Removed unused Parameter (*graphics) out of Parse Function definition i.e
>   virDomainGraphicsListenParseXML() 
> - Corrected code format
> 

Splendid! Except, this is still a diff to your previous patch. And your
previous patch wasn't merged and thus this patch doesn't apply cleanly.
There's no virDomainStorageNetHostSocketValidate() or
virDomainGraphicsListenDefValidate() in current master branch.

>  src/conf/domain_conf.c     |  9 ++---
>  src/conf/domain_validate.c | 68 ++++++++++++++++++++------------------
>  src/conf/domain_validate.h |  4 ---
>  3 files changed, 37 insertions(+), 44 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1e0ac737bb..746bb4efdf 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5836,7 +5836,7 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
>  
>      host->socket = virXMLPropString(hostnode, "socket");
>  
> -    // Socket Validation
> +    /* Socket Validation */
>      if (virDomainStorageNetHostSocketValidate(host, transport) < 0)
>          goto cleanup;

This hunk (and some others) causes this patch doesn't apply cleanly.
Please make sure to send a patch that applies on top of master branch
cleanly:

  https://libvirt.org/submitting-patches.html

Also, this might be worth reading:

  https://libvirt.org/best-practices.html

Looking forward to v3.

Michal

Greetings Sir

My apologies for the incorrect formats as I am a beginner to git. Thank 
you very much for your corrections.

I have a few questions:

Can Questions and doubts be posted within the same mail that consists a 
patch file? Or should they be put in a separate email (as I am 
attempting to do now)?

As per your instruction to make the functions static, I was able to make 
the below function (created by me) static by calling it from 
virDomainGraphicsDefListensValidate() within domain_validate.c

static int
virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
                                    const virDomainGraphicsDef *graphics)

Similarly,
In this patch: Link 
<https://listman.redhat.com/archives/libvir-list/2023-April/239205.html>
I had created a function named;

  int virDomainStorageNetHostSocketValidate(const virStorageNetHostDef *host,
                                      const char *transport);

that removes the validation checks of socket in 
virDomainStorageNetworkParseHost().
While I found a function to call Graphics Validation within validate.c, 
I am unable to find a function that can be used to call 
NetworkHostSocketValidate() from within domain_validate.c. Could you 
please provide a pointer on how it could be done?

Thank you in advance.

Shiva

On 4/5/23 19:56, Shiva wrote:
> Greetings Sir
> 
> My apologies for the incorrect formats as I am a beginner to git. Thank
> you very much for your corrections.

That's okay and GSoC was invented specifically for this reason. To
attract students into Open Source and its development. But at the same
time, review bandwidth is precious and therefore it's important to
listen to feedback. I'm glad to see v2 worked out better.

> 
> I have a few questions:
> 
> Can Questions and doubts be posted within the same mail that consists a
> patch file? Or should they be put in a separate email (as I am
> attempting to do now)?

It's okay to send it as two separate emails even. I mean, usually, when
we work on something and for instance can't make a decision (e.g. on a
design or something), we often just write an e-mail to the list,
explaining what it is we're working on and that we face this decision,
what are the options, etc. And other developers reply with their thoughts.

> 
> As per your instruction to make the functions static, I was able to make
> the below function (created by me) static by calling it from
> virDomainGraphicsDefListensValidate() within domain_validate.c
> 
> static int
> virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
>                                    const virDomainGraphicsDef *graphics)

Yep, that's correct.

> 
> Similarly,
> In this patch: Link
> <https://listman.redhat.com/archives/libvir-list/2023-April/239205.html>
> I had created a function named;
> 
>  int virDomainStorageNetHostSocketValidate(const virStorageNetHostDef *host,
>                                      const char *transport);
> 
> that removes the validation checks of socket in
> virDomainStorageNetworkParseHost().
> While I found a function to call Graphics Validation within validate.c,
> I am unable to find a function that can be used to call
> NetworkHostSocketValidate() from within domain_validate.c. Could you
> please provide a pointer on how it could be done?

Yeah, you may need to write new code. As I've said, we're only gradually
moving code into domain_validate.c. But let me see:

So virDomainStorageNetworkParseHost() is called from exactly two places
(identified by 'git grep -npC10 virDomainStorageNetworkParseHost'):

virDomainBackupDefParseXML
virDomainStorageNetworkParseHosts

Let's focus on the latter for now, we'll get back to the former soon.
virDomainStorageNetworkParseHosts() is then called from the following
places:

virDomainHostdevSubsysSCSIiSCSIDefParseXML
virDomainDiskSourceNetworkParse

after repeating this process more times, we can find what device type
might end up calling this function and what conditions lead to it. For
instance, in this case it's a hostdev that has scsi subdev, iscsi
protocol, etc.

Now, looking at domain_validate.c there's
virDomainDeviceDefValidateInternal() which calls
virDomainHostdevDefValidate() for hostdevs, and then has a block of
checks for scsi subsys, but no further code for iscsi protocol. So that
looks like a place to add the check.

Basically, we're trying to reconstruct the path from domain_conf.c that
lead to the check in domain_validate.c.

And now let's return back to the other caller of the function:
virDomainBackupDefParseXML(). Unfortunately, the XML that's parsed there
is not validated - there's no vir*Validate() function called; so if this
check is removed and moved into domain_validate.c then this path would
be left buggy.

After all, it's probably okay to leave the check there. Introducing
validation to backup XMLs is solvable, but definitely not a bite sized
task for newcomer.

Michal