[PATCH] Conf: Move validation of Graphics Transport and StorageNetwork Socket out of parser
Thank you for the corrections. I understand it now. [1] I have made the Graphics Transport validation function static and moved its call out of the parser, to virDomainGraphicsDefListensValidate() in domain_validate.h. Is this a correct implementation?, it does pass the tests. I have removed the *graphics def parameter from ParseXML function as it became unused after moving the validation out of the Parser. [2] I can't find a way to implement virDomainStorageNetHostSocketValidate() as static. Could you please point as to which function within domain_validate.h could be used to call this validation function? similar to virDomainGraphicsDefListensValidate() for [1]. [3] How do I obtain char *transport within the validation function without parameter passing? It's used only to report this error in the below snippet virReportError(VIR_ERR_XML_ERROR, _("transport '%1$s' does not support socket attribute"), transport); Do I use virXMLPropString(hostnode, "transport") after passing xmlNodePtr hostnode as a parameter? How do I obtain these in domain_validate.h. Thank you for your help! Signed-off-by: K Shiva <shiva_kr@riseup.net> --- 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/4/23 20:10, skran wrote:
Thank you for the corrections. I understand it now. [1] I have made the Graphics Transport validation function static and moved its call out of the parser, to virDomainGraphicsDefListensValidate() in domain_validate.h. Is this a correct implementation?, it does pass the tests. I have removed the *graphics def parameter from ParseXML function as it became unused after moving the validation out of the Parser.
[2] I can't find a way to implement virDomainStorageNetHostSocketValidate() as static. Could you please point as to which function within domain_validate.h could be used to call this validation function? similar to virDomainGraphicsDefListensValidate() for [1].
[3] How do I obtain char *transport within the validation function without parameter passing? It's used only to report this error in the below snippet virReportError(VIR_ERR_XML_ERROR, _("transport '%1$s' does not support socket attribute"), transport); Do I use virXMLPropString(hostnode, "transport") after passing xmlNodePtr hostnode as a parameter? How do I obtain these in domain_validate.h.
This is more correct yes. It moves at least one of the functions to the correct place and calls it from the correct place. But not the other, is there a reason for that? Also, you are still missing couple of points raised earlier - the commit message is rather poor. This is not what anybody would like to see in git log. And since this is the third time I'm raising this objection and it falls flat, I'm given no other option than not review next version if the commit message is not fixed. Sorry. And lastly, we do not send patches against earlier versions of patches. Just send the patch again. What I usually do when given review (assume my patches are on branch "my_fixes": git checkout -b my_fixes_v2 my_fixes git rebase -i master; # here I change all 'pick' to 'edit' (or just 'e') # and change individual commits as requested in # review, and once I'm done: git format-patch -v2 master # if there are two or more patches I also # pass --cover-letter At this point I edit formatted patch(es), in a very careful way: ....
Thank you for your help! Signed-off-by: K Shiva <shiva_kr@riseup.net> ---
... you see these three dashes ^^^? This is the place where I put: This is a v2 of: $(paste url of my previous patch from the libvir-list archive here) diff to v1: - renamed variable in virFunctionXYZ() - fixed memory leak, - other things I've done. Now, if there were more patches than just one, I've covered all of this on the cover letter instead of individual patches. Here's an example of v2 for multiple patches: https://listman.redhat.com/archives/libvir-list/2023-February/237635.html and here's an example of v2 for a single patch: https://listman.redhat.com/archives/libvir-list/2022-December/236463.html Michal
participants (2)
-
Michal Prívozník -
skran