[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/5] util: virbuffer: introduce virBufferEscapeN



On Thu, Feb 23, 2017 at 04:32:32PM +0000, Daniel P. Berrange wrote:
> On Thu, Feb 23, 2017 at 04:26:58PM +0100, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina redhat com>
> > ---
> >  src/libvirt_private.syms |   1 +
> >  src/util/virbuffer.c     | 104 +++++++++++++++++++++++++++++++++++++++++++++++
> >  src/util/virbuffer.h     |   2 +
> >  tests/virbuftest.c       |  41 +++++++++++++++++++
> >  4 files changed, 148 insertions(+)
> > 
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index e9c4d73779..6ce32f1101 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1286,6 +1286,7 @@ virBufferContentAndReset;
> >  virBufferCurrentContent;
> >  virBufferError;
> >  virBufferEscape;
> > +virBufferEscapeN;
> >  virBufferEscapeSexpr;
> >  virBufferEscapeShell;
> >  virBufferEscapeString;
> > diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
> > index d582e7dbec..ad6b29951e 100644
> > --- a/src/util/virbuffer.c
> > +++ b/src/util/virbuffer.c
> > @@ -33,6 +33,7 @@
> >  #include "virbuffer.h"
> >  #include "viralloc.h"
> >  #include "virerror.h"
> > +#include "virstring.h"
> >  
> >  
> >  /* If adding more fields, ensure to edit buf.h to match
> > @@ -588,6 +589,109 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape,
> >      VIR_FREE(escaped);
> >  }
> >  
> > +
> > +struct _virBufferEscapePair {
> > +    char escape;
> > +    char *toescape;
> > +};
> > +
> > +
> > +/**
> > + * virBufferEscapeN:
> > + * @buf: the buffer to append to
> > + * @format: a printf like format string but with only one %s parameter
> > + * @str: the string argument which needs to be escaped
> > + * @...: the variable list of arguments composed
> > + *
> > + * The variable list of arguments @... must be composed of
> > + * 'char escape, char *toescape' pairs followed by NULL.
> 
> So most of the complexity you have here is to deal with the ability
> to turn a single character into a string of escaped characters. Unless
> I'm missing something, you don't actually use that ability in practice.
> If we make it more restrictive so you just have a 1-1 mapping we can
> simplify the API & implementation. eg
> 
>   virBufferEscapeN(virBufferPtr buf,
>                    const char *format,
>                    const char *str,
>                    const char *forbidden,
>                    const char *replacements)
> 
> with strlen(forbidden) == strlen(replacements);

The idea was that a group of characters can be escaped with one character,
so far the usage seems to be really simple.  Currently this would work and
it would be really simple:

    virBufferEscapeN(buf, "%s", str, ",=", ",\\");

but imagine this scenario:

    virBufferEscapeN(buf, "%s", str, ",=%&*()", ",\\\\\\\\\\\\");

    virBufferEscapeN(buf, "%s", str, ',', ",", '\\', "=%&*()", NULL);

I think that the second approach is better and easier for user of
virBufferEscapeN.
 
> > + *
> > + * This has the same functionality as virBufferEscape with the extension
> > + * that allows to specify multiple pairs of chars that needs to be escaped.
> > + */
> > +void
> > +virBufferEscapeN(virBufferPtr buf,
> > +                 const char *format,
> > +                 const char *str,
> > +                 ...)
> > +{
> > +    int len;
> > +    size_t i;
> > +    char escape;
> > +    char *toescape;
> > +    char *toescapeall = NULL;
> > +    char *escaped = NULL;
> > +    char *out;
> > +    const char *cur;
> > +    struct _virBufferEscapePair escapeItem;
> > +    struct _virBufferEscapePair *escapeList = NULL;
> > +    size_t nescapeList = 0;
> > +    va_list ap;
> > +
> > +    if ((format == NULL) || (buf == NULL) || (str == NULL))
> > +        return;
> > +
> > +    if (buf->error)
> > +        return;
> > +
> > +    va_start(ap, str);
> > +
> > +    while ((escape = va_arg(ap, int))) {
> > +        if (!(toescape = va_arg(ap, char *))) {
> > +            virBufferSetError(buf, errno);
> > +            goto cleanup;
> > +        }
> > +
> > +        escapeItem.escape = escape;
> > +        escapeItem.toescape = toescape;
> > +
> > +        if (VIR_STRCAT_QUIET(toescapeall, toescape) < 0) {
> > +            virBufferSetError(buf, errno);
> > +            goto cleanup;
> > +        }
> > +
> > +        if (VIR_APPEND_ELEMENT_QUIET(escapeList, nescapeList, escapeItem) < 0) {
> > +            virBufferSetError(buf, errno);
> > +            goto cleanup;
> > +        }
> > +    }
> 
> This whole loop would go away with the simpler API contract, which
> nicely avoids doing allocations in the loop body.
> 
> 
> > +    len = strlen(str);
> > +    if (strcspn(str, toescapeall) == len) {
> > +        virBufferAsprintf(buf, format, str);
> > +        goto cleanup;
> > +    }
> > +
> > +    if (xalloc_oversized(2, len) ||
> > +        VIR_ALLOC_N_QUIET(escaped, 2 * len + 1) < 0) {
> > +        virBufferSetError(buf, errno);
> > +        goto cleanup;
> > +    }
> > +
> > +    cur = str;
> > +    out = escaped;
> > +    while (*cur != 0) {
> > +        for (i = 0; i < nescapeList; i++) {
> > +            if (strchr(escapeList[i].toescape, *cur)) {
> > +                *out++ = escapeList[i].escape;
> > +                break;
> > +            }
> > +        }
> > +        *out++ = *cur;
> > +        cur++;
> > +    }
> > +    *out = 0;
> > +
> > +    virBufferAsprintf(buf, format, escaped);
> > +
> > + cleanup:
> > +    va_end(ap);
> > +    VIR_FREE(toescapeall);
> > +    VIR_FREE(escapeList);
> > +    VIR_FREE(escaped);
> > +}
> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]