[PATCH v2] Conf: Move validation of virDomainGraphicsListenDef out of Parser
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
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
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
participants (3)
-
K Shiva -
Michal Prívozník -
Shiva