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

Nicolas George nicolas.george at normalesup.org
Fri Dec 21 22:23:16 CET 2012


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?

> +
>  /**
>   * @}
>   */
> 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.

> +        }
> +
> +        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.

> +            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.

> + */
> +int av_bprint_escape(AVBPrint *dstbuf, const char *src, const char *special_chars,
> +                     enum AVEscapeMode mode);
> +
>  #endif /* AVUTIL_BPRINT_H */
> diff --git a/tools/ffescape.c b/tools/ffescape.c
> index d777fe4..6291547 100644
> --- a/tools/ffescape.c
> +++ b/tools/ffescape.c
> @@ -51,71 +51,6 @@ static void usage(void)
>             "-s SPECIAL_CHARS  set the list of special characters\n");
>  }
>  
> -#define WHITESPACES " \n\t"
> -
> -enum EscapeMode {
> -    ESCAPE_MODE_FULL,
> -    ESCAPE_MODE_LAZY,
> -    ESCAPE_MODE_QUOTE,
> -};
> -
> -static int escape(char **dst, const char *src, const char *special_chars,
> -                  enum EscapeMode mode)
> -{
> -    AVBPrint dstbuf;
> -
> -    av_bprint_init(&dstbuf, 1, AV_BPRINT_SIZE_UNLIMITED);
> -
> -    switch (mode) {
> -    case ESCAPE_MODE_FULL:
> -    case ESCAPE_MODE_LAZY:
> -        /* \-escape characters */
> -
> -        if (mode == ESCAPE_MODE_LAZY && strchr(WHITESPACES, *src))
> -            av_bprintf(&dstbuf, "\\%c", *src++);
> -
> -        for (; *src; src++) {
> -            if ((special_chars && strchr(special_chars, *src)) ||
> -                strchr("'\\", *src) ||
> -                (mode == ESCAPE_MODE_FULL && strchr(WHITESPACES, *src)))
> -                av_bprintf(&dstbuf, "\\%c", *src);
> -            else
> -                av_bprint_chars(&dstbuf, *src, 1);
> -        }
> -
> -        if (mode == ESCAPE_MODE_LAZY && strchr(WHITESPACES, dstbuf.str[dstbuf.len-1])) {
> -            char c = dstbuf.str[dstbuf.len-1];
> -            dstbuf.str[dstbuf.len-1] = '\\';
> -            av_bprint_chars(&dstbuf, c, 1);
> -        }
> -        break;
> -
> -    case 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);
> -    }
> -
> -    if (!av_bprint_is_complete(&dstbuf)) {
> -        av_bprint_finalize(&dstbuf, NULL);
> -        return AVERROR(ENOMEM);
> -    } else {
> -        av_bprint_finalize(&dstbuf, dst);
> -        return 0;
> -    }
> -}
> -
>  int main(int argc, char **argv)
>  {
>      AVBPrint src;
> @@ -123,7 +58,7 @@ int main(int argc, char **argv)
>      const char *outfilename = NULL, *infilename = NULL;
>      FILE *outfile = NULL, *infile = NULL;
>      const char *prompt = "=> ";
> -    enum EscapeMode escape_mode = ESCAPE_MODE_LAZY;
> +    enum AVEscapeMode escape_mode = AV_ESCAPE_MODE_LAZY;
>      int level = 1;
>      int echo = 0;
>      char *special_chars = NULL;
> @@ -154,9 +89,9 @@ int main(int argc, char **argv)
>              break;
>          }
>          case 'm':
> -            if      (!strcmp(optarg, "full"))  escape_mode = ESCAPE_MODE_FULL;
> -            else if (!strcmp(optarg, "lazy"))  escape_mode = ESCAPE_MODE_LAZY;
> -            else if (!strcmp(optarg, "quote")) escape_mode = ESCAPE_MODE_QUOTE;
> +            if      (!strcmp(optarg, "full"))  escape_mode = AV_ESCAPE_MODE_FULL;
> +            else if (!strcmp(optarg, "lazy"))  escape_mode = AV_ESCAPE_MODE_LAZY;
> +            else if (!strcmp(optarg, "quote")) escape_mode = AV_ESCAPE_MODE_QUOTE;
>              else {
>                  av_log(NULL, AV_LOG_ERROR,
>                         "Invalid value '%s' for option -m, "
> @@ -219,7 +154,7 @@ int main(int argc, char **argv)
>      /* escape */
>      dst_buf = src_buf;
>      while (level--) {
> -        if (escape(&dst_buf, src_buf, special_chars, escape_mode) < 0) {
> +        if (av_escape(&dst_buf, src_buf, special_chars, escape_mode) < 0) {
>              av_log(NULL, AV_LOG_ERROR, "Could not escape string\n");
>              return 1;
>          }
> -- 
> 1.7.9.5
> 

Thanks for the work, and sorry for the delay.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121221/43410c03/attachment.asc>


More information about the ffmpeg-devel mailing list