Signed-off-by: Shi Lei <shi_lei@massclouds.com>
---
 src/conf/network_conf.c | 4 ++--
 src/conf/network_conf.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 964a8a7..b807bac 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2280,8 +2280,8 @@ virNetworkDNSDefFormat(virBufferPtr buf,
     }
 
     for (i = 0; i < def->ntxts; i++) {
-        virBufferEscapeString(buf, "<txt name='%s' ", def->txts[i].name);
-        virBufferEscapeString(buf, "value='%s'/>\n", def->txts[i].value);
+        if (virNetworkDNSTxtDefFormatBuf(buf, "txt", &def->txts[i], NULL) < 0)
+            return -1;
     }
 
     for (i = 0; i < def->nsrvs; i++) {
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index eac8a76..b3c2895 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -130,7 +130,7 @@ struct _virNetworkDHCPHostDef {
 
 typedef struct _virNetworkDNSTxtDef virNetworkDNSTxtDef;
 typedef virNetworkDNSTxtDef *virNetworkDNSTxtDefPtr;
-struct _virNetworkDNSTxtDef {   /* genparse:concisehook */
+struct _virNetworkDNSTxtDef {   /* genparse:concisehook, genformat */
     char *name;                 /* xmlattr, required */
     char *value;                /* xmlattr */
 };
-- 
2.17.1

On Wed, Jun 10, 2020 at 09:20:36AM +0800, Shi Lei wrote:
> Signed-off-by: Shi Lei <shi_lei@massclouds.com>
> ---
>  src/conf/network_conf.c | 4 ++--
>  src/conf/network_conf.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 964a8a7..b807bac 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -2280,8 +2280,8 @@ virNetworkDNSDefFormat(virBufferPtr buf,
>      }
>  
>      for (i = 0; i < def->ntxts; i++) {
> -        virBufferEscapeString(buf, "<txt name='%s' ", def->txts[i].name);
> -        virBufferEscapeString(buf, "value='%s'/>\n", def->txts[i].value);
> +        if (virNetworkDNSTxtDefFormatBuf(buf, "txt", &def->txts[i], NULL) < 0)
> +            return -1;
>      }

For sake of review, the new code looks like this:

int
virNetworkDNSTxtDefFormatBuf(virBufferPtr buf,
                             const char *name,
                             const virNetworkDNSTxtDef *def,
                             void *opaque)
{
    VIR_USED(opaque);

    if (!def)
        return 0;

    if (!(def->name || def->value))
        return 0;

    virBufferAsprintf(buf, "<%s", name);

    if (def->name)
        virBufferAsprintf(buf, " name='%s'", def->name);

    if (def->value)
        virBufferAsprintf(buf, " value='%s'", def->value);

    virBufferAddLit(buf, "/>\n");

    return 0;
}

This is a lot longer, but obviously the code is more general
purpose.

I'm not sure why we need to pass "txt" into this method though.
Can't we just hardcode "<txt" instead of formatting "<%s", name ?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, Jun 29, 2020 at 13:52:51 +0100, Daniel Berrange wrote:
> On Wed, Jun 10, 2020 at 09:20:36AM +0800, Shi Lei wrote:
> > Signed-off-by: Shi Lei <shi_lei@massclouds.com>
> > ---
> >  src/conf/network_conf.c | 4 ++--
> >  src/conf/network_conf.h | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> > index 964a8a7..b807bac 100644
> > --- a/src/conf/network_conf.c
> > +++ b/src/conf/network_conf.c
> > @@ -2280,8 +2280,8 @@ virNetworkDNSDefFormat(virBufferPtr buf,
> >      }
> >  
> >      for (i = 0; i < def->ntxts; i++) {
> > -        virBufferEscapeString(buf, "<txt name='%s' ", def->txts[i].name);
> > -        virBufferEscapeString(buf, "value='%s'/>\n", def->txts[i].value);
> > +        if (virNetworkDNSTxtDefFormatBuf(buf, "txt", &def->txts[i], NULL) < 0)
> > +            return -1;
> >      }
> 
> For sake of review, the new code looks like this:
> 
> int
> virNetworkDNSTxtDefFormatBuf(virBufferPtr buf,
>                              const char *name,
>                              const virNetworkDNSTxtDef *def,
>                              void *opaque)
> {
>     VIR_USED(opaque);
> 
>     if (!def)
>         return 0;
> 
>     if (!(def->name || def->value))
>         return 0;
> 
>     virBufferAsprintf(buf, "<%s", name);
> 
>     if (def->name)
>         virBufferAsprintf(buf, " name='%s'", def->name);

Specifically, these are wrong as they don't use virBufferEscapeString
for formatting an XML thus the string won't have XML entities escaped.

Looks like this must be applied everywhere where the string comes from
the user.

On Mon, Jun 29, 2020 at 03:13:30PM +0200, Peter Krempa wrote:
> On Mon, Jun 29, 2020 at 13:52:51 +0100, Daniel Berrange wrote:
> > On Wed, Jun 10, 2020 at 09:20:36AM +0800, Shi Lei wrote:
> > > Signed-off-by: Shi Lei <shi_lei@massclouds.com>
> > > ---
> > >  src/conf/network_conf.c | 4 ++--
> > >  src/conf/network_conf.h | 2 +-
> > >  2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> > > index 964a8a7..b807bac 100644
> > > --- a/src/conf/network_conf.c
> > > +++ b/src/conf/network_conf.c
> > > @@ -2280,8 +2280,8 @@ virNetworkDNSDefFormat(virBufferPtr buf,
> > >      }
> > >  
> > >      for (i = 0; i < def->ntxts; i++) {
> > > -        virBufferEscapeString(buf, "<txt name='%s' ", def->txts[i].name);
> > > -        virBufferEscapeString(buf, "value='%s'/>\n", def->txts[i].value);
> > > +        if (virNetworkDNSTxtDefFormatBuf(buf, "txt", &def->txts[i], NULL) < 0)
> > > +            return -1;
> > >      }
> > 
> > For sake of review, the new code looks like this:
> > 
> > int
> > virNetworkDNSTxtDefFormatBuf(virBufferPtr buf,
> >                              const char *name,
> >                              const virNetworkDNSTxtDef *def,
> >                              void *opaque)
> > {
> >     VIR_USED(opaque);
> > 
> >     if (!def)
> >         return 0;
> > 
> >     if (!(def->name || def->value))
> >         return 0;
> > 
> >     virBufferAsprintf(buf, "<%s", name);
> > 
> >     if (def->name)
> >         virBufferAsprintf(buf, " name='%s'", def->name);
> 
> Specifically, these are wrong as they don't use virBufferEscapeString
> for formatting an XML thus the string won't have XML entities escaped.
> 
> Looks like this must be applied everywhere where the string comes from
> the user.

With hand written code we've tried to optimize where we do escaping, but
really we should just be escaping pretty much all string values. The only
reasonable place to omit escaping is if dealing with a string that came
from an enum conversion. So I think the code generator to just do full
escaping, unless it is easy for the generator to skip with enums.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|