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

Stefano Sabatini stefasab at gmail.com
Tue Jan 8 00:49:28 CET 2013


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.

Aligned.

> 
> > + */
> > +int av_escape(char **dst, const char *src, const char *special_chars,
> > +              enum AVEscapeMode mode);
> 
> Should we consider making backslash an argument?

I prefer not at this stage, as I already argumented.
 
> > +
> >  /**
> >   * @}
> >   */
> > diff --git a/libavutil/bprint.c b/libavutil/bprint.c
> > index 4684ab4..fb3630e 100644
> > --- a/libavutil/bprint.c
> > +++ b/libavutil/bprint.c
> > @@ -23,6 +23,7 @@
> >  #include <string.h>
> >  #include <time.h>
> >  #include "avassert.h"
> > +#include "avstring.h"
> >  #include "bprint.h"
> >  #include "common.h"
> >  #include "error.h"
> > @@ -217,6 +218,56 @@ int av_bprint_finalize(AVBPrint *buf, char **ret_str)
> >      return ret;
> >  }
> >  
> > +#define WHITESPACES " \n\t"
> > +
> > +int av_bprint_escape(AVBPrint *dstbuf, const char *src, const char *special_chars,
> > +                     enum AVEscapeMode mode)
> > +{
> > +    switch (mode) {
> > +    case AV_ESCAPE_MODE_FULL:
> > +    case AV_ESCAPE_MODE_LAZY:
> > +        /* \-escape characters */
> > +
> > +        if (mode == AV_ESCAPE_MODE_LAZY && strchr(WHITESPACES, *src))
> > +            av_bprintf(dstbuf, "\\%c", *src++);
> > +
> > +        for (; *src; src++) {
> > +            if ((special_chars && strchr(special_chars, *src)) ||
> > +                strchr("'\\", *src) ||
> > +                (mode == AV_ESCAPE_MODE_FULL && strchr(WHITESPACES, *src)))
> 
> > +                av_bprintf(dstbuf, "\\%c", *src);
> > +            else
> > +                av_bprint_chars(dstbuf, *src, 1);
> 
> Could be simplified by using this in the true branch of the if:
> 
>     av_bprint_chars(dstbuf, '\\', 1);
> 
> and making the else part unconditional.

Done.
 
> > +        }
> > +
> > +        if (mode == AV_ESCAPE_MODE_LAZY &&
> > +            strchr(WHITESPACES, dstbuf->str[dstbuf->len-1])) {
> 
> You have to check that dstbuf->len > 0. Also, you have to check that it is
> less than dstbuf->size, in case truncation happened.
> 
> Also, it will not work if a space is present in special_chars.
> 
> I believe the best way to simplify this would be to avoid going back for the
> last char.

Changed.
 
> > +            char c = dstbuf->str[dstbuf->len-1];
> > +            dstbuf->str[dstbuf->len-1] = '\\';
> > +            av_bprint_chars(dstbuf, c, 1);
> > +        }
> > +        break;
> > +
> > +    case AV_ESCAPE_MODE_QUOTE:
> > +        /* enclose between '' the string */
> > +        av_bprint_chars(dstbuf, '\'', 1);
> > +        for (; *src; src++) {
> > +            if (*src == '\'')
> > +                av_bprintf(dstbuf, "'\\''");
> > +            else
> > +                av_bprint_chars(dstbuf, *src, 1);
> > +        }
> > +        av_bprint_chars(dstbuf, '\'', 1);
> > +        break;
> > +
> > +    default:
> > +        /* unknown escape mode */
> > +        return AVERROR(EINVAL);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  #ifdef TEST
> >  
> >  #undef printf
> > diff --git a/libavutil/bprint.h b/libavutil/bprint.h
> > index f3915fe..3506d7c 100644
> > --- a/libavutil/bprint.h
> > +++ b/libavutil/bprint.h
> > @@ -22,6 +22,7 @@
> >  #define AVUTIL_BPRINT_H
> >  
> >  #include "attributes.h"
> > +#include "avstring.h"
> >  
> >  /**
> >   * Define a structure with extra padding to a fixed size
> > @@ -180,4 +181,17 @@ static inline int av_bprint_is_complete(AVBPrint *buf)
> >   */
> >  int av_bprint_finalize(AVBPrint *buf, char **ret_str);
> >  
> > +/**
> > + * Escape the content in src and append it to dstbuf.
> > + *
> > + * @param dstbuf already inited destination bprint buffer
> > + * @param src string containing the text to escape
> > + * @param special_chars list of special characters to escape
> > + * @param mode escape mode to employ
> > + * @return >= 0 in case of success, a negative value in case of error
> > + * @see av_escape()
> 
> Ditto about alignment, of course.

Fixed.

[...] 
> Thanks for the work, and sorry for the delay.

Updated.
-- 
FFmpeg = Fascinating & Fancy Magical Powerful Embarassing Governor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavu-add-escape-API.patch
Type: text/x-diff
Size: 12141 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130108/6d28b200/attachment.bin>


More information about the ffmpeg-devel mailing list