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(a)redhat.com&gt; > > --- > > 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.