[PATCH v2 13/19] Refactoring virDomainChrSourceDefParseXML() to use XPath

[Michal Prívozník]

On 5/4/21 1:40 PM, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova <khanicov at redhat.com>
> ---
>  src/conf/domain_conf.c | 190 ++++++++++++++++++++---------------------
>  1 file changed, 94 insertions(+), 96 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5ac15fe9e8..c5b13783f3 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11350,112 +11350,112 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def,
>                                virDomainChrDef *chr_def,
>                                xmlXPathContextPtr ctxt)
>  {
> -    bool logParsed = false;
> -    bool protocolParsed = false;
> -    int sourceParsed = 0;
> +    xmlNodePtr log = NULL;
> +    xmlNodePtr protocol = NULL;
> +    g_autofree xmlNodePtr *source = NULL;
> +    int n = 0;
> +    VIR_XPATH_NODE_AUTORESTORE(ctxt)
>  
> -    for (; cur; cur = cur->next) {
> -        if (cur->type != XML_ELEMENT_NODE)
> -            continue;
> +    ctxt->node = cur;
>  
> -        if (virXMLNodeNameEqual(cur, "source")) {
> -            /* Parse only the first source element since only one is used
> -             * for chardev devices, the only exception is UDP type, where
> -             * user can specify two source elements. */
> -            if (sourceParsed >= 1 && def->type != VIR_DOMAIN_CHR_TYPE_UDP) {
> -                virReportError(VIR_ERR_XML_ERROR, "%s",
> -                               _("only one source element is allowed for "
> -                                 "character device"));
> -                goto error;
> -            } else if (sourceParsed >= 2) {
> -                virReportError(VIR_ERR_XML_ERROR, "%s",
> -                               _("only two source elements are allowed for "
> -                                 "character device"));
> -                goto error;
> -            }
> -            sourceParsed++;
> +    if ((n = virXPathNodeSet("./source", ctxt, &source)) > 0) {
> +        /* Parse only the first source element since only one is used
> +         * for chardev devices, the only exception is UDP type, where
> +         * user can specify two source elements. */
> +        if (n > 1 && def->type != VIR_DOMAIN_CHR_TYPE_UDP) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("only one source element is allowed for "
> +                             "character device"));
> +            goto error;
> +        } else if (n > 2) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("only two source elements are allowed for "
> +                             "character device"));
> +            goto error;
> +        }
>  
> -            switch ((virDomainChrType) def->type) {
> -            case VIR_DOMAIN_CHR_TYPE_FILE:
> -                if (virDomainChrSourceDefParseFile(def, cur) < 0)
> -                    goto error;
> -                break;
> +        switch ((virDomainChrType) def->type) {
> +        case VIR_DOMAIN_CHR_TYPE_FILE:
> +            if (virDomainChrSourceDefParseFile(def, source[0]) < 0)
> +                goto error;
> +            break;
>  
> -            case VIR_DOMAIN_CHR_TYPE_PTY:
> -                /* PTY path is only parsed from live xml.  */
> -                if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
> -                    def->data.file.path = virXMLPropString(cur, "path");
> -                break;
> +        case VIR_DOMAIN_CHR_TYPE_PTY:
> +            /* PTY path is only parsed from live xml.  */
> +            if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
> +                def->data.file.path = virXPathString("string(./source/@path)", ctxt);
> +            break;
>  
> -            case VIR_DOMAIN_CHR_TYPE_DEV:
> -            case VIR_DOMAIN_CHR_TYPE_PIPE:
> -                def->data.file.path = virXMLPropString(cur, "path");
> -                break;
> +        case VIR_DOMAIN_CHR_TYPE_DEV:
> +        case VIR_DOMAIN_CHR_TYPE_PIPE:
> +            def->data.file.path = virXPathString("string(./source/@path)", ctxt);
> +            break;
>  
> -            case VIR_DOMAIN_CHR_TYPE_UNIX:
> -                if (virDomainChrSourceDefParseUnix(def, cur, ctxt) < 0)
> -                    goto error;
> -                break;
> +        case VIR_DOMAIN_CHR_TYPE_UNIX:
> +            if (virDomainChrSourceDefParseUnix(def, source[0], ctxt) < 0)
> +                goto error;
> +            break;
>  
> -            case VIR_DOMAIN_CHR_TYPE_UDP:
> -                if (virDomainChrSourceDefParseUDP(def, cur) < 0)
> +        case VIR_DOMAIN_CHR_TYPE_UDP:
> +            if ((virDomainChrSourceDefParseUDP(def, source[0]) < 0) ||
> +                (source[1] && virDomainChrSourceDefParseUDP(def, source[1]) < 0))

Ooops. While this may work by chance, it's not correct.
virXPathNodeSet() does not return a NULL terminated array. Therefore, if
n == 1, then source[0] is allocated, but source[1] is not. Fortunately,
the fix is as easy as s/source[1]/n == 2/.

>                      goto error;
> -                break;
> +            break;

Michal