[FFmpeg-devel] [PATCH] lavu: add escape API

[Stefano Sabatini]

On date Friday 2012-12-21 22:23:16 +0100, Nicolas George encoded:
> Le primidi 1er nivôse, an CCXXI, Stefano Sabatini a écrit :
> > The escape API will be useful to perform escaping programmatically, which
> > is required when crafting argument strings, and will be used for context
> > printing as well.
> 
> I believe the API is useful, but I also believe it is very tricky to design
> properly. My guess is that we will get it wrong the first time anyway.
> 
> Note: I am considering extending av_get_token() to make the escaping more
> practical. One of the changes I am considering, but am not entirely sure
> about, is to ignore delimiter chars if there is an unclosed parenthesis,
> brace or bracket: the plus side is that it reduces the need for escaping in
> expressions, the downside is that unmatched parenthesis need to be escaped.
> 
> The remarks below have these extensions in mind.
> 
> > 
> > TODO: bump minor, add APIchanges entry
> > ---
> >  libavutil/avstring.c |   18 ++++++++++++
> >  libavutil/avstring.h |   17 ++++++++++++
> >  libavutil/bprint.c   |   51 ++++++++++++++++++++++++++++++++++
> >  libavutil/bprint.h   |   14 ++++++++++
> >  tools/ffescape.c     |   75 ++++----------------------------------------------
> >  5 files changed, 105 insertions(+), 70 deletions(-)
> > 
> > diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> > index f03df67..b2af333 100644
> > --- a/libavutil/avstring.c
> > +++ b/libavutil/avstring.c
> > @@ -26,6 +26,7 @@
> >  #include <ctype.h>
> >  #include "avstring.h"
> >  #include "mem.h"
> > +#include "bprint.h"
> >  
> >  int av_strstart(const char *str, const char *pfx, const char **ptr)
> >  {
> > @@ -211,6 +212,23 @@ int av_strncasecmp(const char *a, const char *b, size_t n)
> >      return c1 - c2;
> >  }
> >  
> > +int av_escape(char **dst, const char *src, const char *special_chars,
> > +              enum AVEscapeMode mode)
> > +{
> > +    AVBPrint dstbuf;
> > +
> > +    av_bprint_init(&dstbuf, 1, AV_BPRINT_SIZE_UNLIMITED);
> > +
> > +    av_bprint_escape(&dstbuf, src, special_chars, mode);
> > +    if (!av_bprint_is_complete(&dstbuf)) {
> > +        av_bprint_finalize(&dstbuf, NULL);
> > +        return AVERROR(ENOMEM);
> > +    } else {
> > +        av_bprint_finalize(&dstbuf, dst);
> > +        return 0;
> > +    }
> > +}
> > +
> >  #ifdef TEST
> >  
> >  #include "common.h"
> > diff --git a/libavutil/avstring.h b/libavutil/avstring.h
> > index f73d6e7..2671506 100644
> > --- a/libavutil/avstring.h
> > +++ b/libavutil/avstring.h
> > @@ -202,6 +202,23 @@ int av_strcasecmp(const char *a, const char *b);
> >   */
> >  int av_strncasecmp(const char *a, const char *b, size_t n);
> >  
> > +enum AVEscapeMode {
> > +    AV_ESCAPE_MODE_FULL,   ///< escape each single special character and whitespace by prefixing '\'
> > +    AV_ESCAPE_MODE_LAZY,   ///< as AV_ESCAPE_MODE_FULL, but only escape whitespaces at the begin and at the end of the string
> > +    AV_ESCAPE_MODE_QUOTE,  ///< perform quote-escaping
> 
> I do not find the docs very clear.
> 
> Also, I wonder whether flags would not be better than modes. At the very
> least they are more extensible. Something like this come to mind:
> 
> /**
>  * Control the escaping choices.
>  * By default, try to produce a "good looking" string that can be parsed
>  * back by av_get_token().
>  */
> enum AVEscapeFlags {
> 
>     /**
>      * Use backslash escaping only.
>      */
>     AV_ESCAPE_USE_BACKSLASH = 0x1,
> 
>     /**
>      * Use single-quote escaping only.
>      */
>     AV_ESCAPE_USE_SINGLE_QUOTE = 0x2,
> 
>     /* note: the gap between 0x2 and 0x100 can be used to add other USE
>      * flags; they are not really flags, as they are exclusive */
> 
>     /**
>      * Consider spaces special and escape them even in the middle of the
>      * string.
>      * This is equivalent to adding the whitespace characters to the special
>      * characters lists, except it is guaranteed to use the exact same list
>      * of whitespace characters as the rest of libavutil.
>      */
>     AV_ESCAPE_WHITESPACE = 0x100,
> 
>     /**
>      * Escape only specified special characters.
>      * Without this flag, escape also any characters that may be considered
>      * special by av_get_token(), such as the single quote.
>      */
>     AV_ESCAPE_STRICT = 0x200,
> 
> };
> 
> > +};
> > +
> > +/**
> > + * Escape string in src, and put the escaped string in an allocated
> > + * string in *dst, which must be freed with av_free().
> > + *
> > + * @param special_chars string containing the special characters which need to be escaped
> > + * @param mode escape mode to employ
> > + * @return >=0 in case of success, or a negative error code in case of error
> 
> I find it more readable when the explanations are aligned together.
> 
> > + */
> > +int av_escape(char **dst, const char *src, const char *special_chars,
> > +              enum AVEscapeMode mode);
> 
> Should we consider making backslash an argument?

My purpose was to implement a possibly *simple* escaping function,
which performs the inverse operation of av_get_token(), with
\-escaping and '-quoting.

It is not my plan to provide a generic function for all possible types
of escaping (whose effective usefulness in libavutil is arguable). In
that case a different design would be needed (escape algorithm context
and private options) but I think that would be overkill.

So this is my proposal:

enum AVEscapeMode {
    AV_ESCAPE_USE_BACKSLASH    = 1,  ///< Use backslash escaping only.
    AV_ESCAPE_USE_SINGLE_QUOTE = 2,  ///< Use single-quote escaping only.
};

/**
 * Consider spaces special and escape them even in the middle of the
 * string.
 * This is equivalent to adding the whitespace characters to the special
 * characters lists, except it is guaranteed to use the exact same list
 * of whitespace characters as the rest of libavutil.
 */
#define AV_ESCAPE_FLAG_WHITESPACE = 0x01,

/**
 * Escape only specified special characters.
 * Without this flag, escape also any characters that may be considered
 * special by av_get_token(), such as the single quote.
 */
#define AV_ESCAPE_FLAG_STRICT = 0x02,
};

int av_escape(char **dst, const char *src, const char *special_chars,
              enum AVEscapeMode mode, int flags);

I'm not sure about the utility of AV_ESCAPE_FLAG_STRICT, but this API
should be extensible enough in case we decide to extend
av_get_token().
-- 
FFmpeg = Fanciful and Fiendish Maxi Pacific Extended Gangster