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
