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

Nicolas George nicolas.george at normalesup.org
Thu Jan 24 18:01:31 CET 2013


Le duodi 22 nivôse, an CCXXI, Stefano Sabatini a écrit :
> > av_bprint_finalize will fail if the buffer is truncated, so you do not have
> > to check for it.
> Not really from what I see.

You are right. I wonder if fixing it would be a good idea.

> What I'm trying to avoid is to make as hard as possible for the user
> to get it wrong, the price to pay is a more complicated interface so
> it is a tradeoff matter. I'll reconsider to merge the two fields.

My concern is that by adding a mode argument, you force the API user to make
a choice, even if they do not want to because they only want whatever
escaping lavu considers best.

> Truncation error is not considered an error here, invalid parameters
> yes.

I consider there are two kinds of errors: those that can happen in a correct
programs and the bugs, and I consider that the treatment for the two kinds
of errors can be different.

Truncation of a bprint buffer can happen in a 100% correct program because
it depends on outside conditions: memory availability. Therefore, the API
must provide a means to check it.

Invalid flags, OTOH, are a programming error, a bug.

IMHO, the way of returning both kind of errors should be different; the
simple C API does not allow that easily. But when a function can only fail
due to a bug, never due to environmental circumstances, I find it better
that it returns nothing. Reporting the bugs can be done another way, such as
an assert failure or a log message.

In this particular case, I would find this completely acceptable:

	default:
	    av_bprintf(dstbuf, "*** INVALID av_quote MODE %x ***", mode);
	    return;

But the best solution would still be to have no failure cases, i.e. have all
flags configurations acceptable. For example, in the doxy:

	* Any other value for mode will be considered equivalent to
	* AV_ESCAPE_MODE_BACKSLASH, but this behaviour can change without
	* notice.

>	Also in case more complex operations are performed (e.g. alloc)
> it would be handy to be able to return an error.

The principle in that case is to place the bprint buffer in a truncated
state, so that the error can be detected at the end.

> >From f0d0f51ab59a0eaa2e27838afae963c96c1b02f1 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefasab at gmail.com>
> Date: Sun, 16 Dec 2012 12:17:23 +0100
> Subject: [PATCH] lavu: add escape API
> 
> 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.
> 
> This is based on the ffescape tool code, with a few extensions and fixes.
> 
> TODO: bump minor, add APIchanges entry
> ---
>  libavutil/avstring.c |   19 +++++++++++
>  libavutil/avstring.h |   37 ++++++++++++++++++++
>  libavutil/bprint.c   |   47 ++++++++++++++++++++++++++
>  libavutil/bprint.h   |   15 ++++++++
>  tools/ffescape.c     |   92 +++++++++++---------------------------------------
>  5 files changed, 137 insertions(+), 73 deletions(-)
> 
> diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> index 7072a55..2ccb581 100644
> --- a/libavutil/avstring.c
> +++ b/libavutil/avstring.c
> @@ -28,6 +28,7 @@
>  #include "config.h"
>  #include "common.h"
>  #include "mem.h"
> +#include "bprint.h"
>  
>  int av_strstart(const char *str, const char *pfx, const char **ptr)
>  {
> @@ -251,6 +252,24 @@ const char *av_dirname(char *path)
>      return path;
>  }
>  
> +int av_escape(char **dst, const char *src, const char *special_chars,
> +              enum AVEscapeMode mode, int flags)
> +{
> +    AVBPrint dstbuf;
> +    int ret;
> +
> +    av_bprint_init(&dstbuf, 1, AV_BPRINT_SIZE_UNLIMITED);
> +    if ((ret = av_bprint_escape(&dstbuf, src, special_chars, mode, flags)) < 0)
> +        return ret;
> +
> +    if (!av_bprint_is_complete(&dstbuf)) {
> +        av_bprint_finalize(&dstbuf, NULL);
> +        return AVERROR(ENOMEM);
> +    } else {
> +        av_bprint_finalize(&dstbuf, dst);
> +        return dstbuf.len;
> +    }
> +}
>  
>  #ifdef TEST
>  
> diff --git a/libavutil/avstring.h b/libavutil/avstring.h
> index d7af9ec..0514584 100644
> --- a/libavutil/avstring.h
> +++ b/libavutil/avstring.h
> @@ -218,6 +218,43 @@ const char *av_basename(const char *path);
>   */
>  const char *av_dirname(char *path);
>  
> +enum AVEscapeMode {
> +    AV_ESCAPE_MODE_BACKSLASH = 1, ///< Use backslash escaping.
> +    AV_ESCAPE_MODE_QUOTE     = 2, ///< Use single-quote escaping.
> +};
> +
> +/**
> + * 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
> +
> +/**
> + * Escape string in src, and put the escaped string in an allocated
> + * string in *dst, which must be freed with av_free().
> + *
> + * @param dst           pointer where an allocated string is put
> + * @param src           string to escape, must be non-NULL
> + * @param special_chars string containing the special characters which need to be escaped
> + * @param mode          escape mode to employ, see AV_ESCAPE_MODE_ macros
> + * @param flags         flags which control how to escape, see AV_ESCAPE_FLAG_ macros
> + * @return the length of the allocated string, or a negative error code in case of error
> + * @see av_bprint_escape()
> + */
> +int av_escape(char **dst, const char *src, const char *special_chars,
> +              enum AVEscapeMode mode, int flags);
> +
>  /**
>   * @}
>   */
> diff --git a/libavutil/bprint.c b/libavutil/bprint.c
> index 4684ab4..f70f76f 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,52 @@ 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, int flags)
> +{
> +    const char *src0 = src;
> +
> +    switch (mode) {
> +    case AV_ESCAPE_MODE_BACKSLASH:
> +        /* \-escape characters */

> +        for (; *src; src++) {
> +            int is_first_last       = src == src0 || !*(src+1);
> +            int is_ws               = !!strchr(WHITESPACES, *src);
> +            int is_strictly_special = special_chars && strchr(special_chars, *src);
> +            int is_special          =
> +                is_strictly_special || strchr("'\\", *src) ||
> +                (is_ws && (flags & AV_ESCAPE_FLAG_WHITESPACE));
> +
> +            if (is_strictly_special ||
> +                (!(flags & AV_ESCAPE_FLAG_STRICT) &&
> +                 (is_special || (is_ws && is_first_last))))
> +                av_bprint_chars(dstbuf, '\\', 1);
> +            av_bprint_chars(dstbuf, *src, 1);

Looks nice. Did you look at the assembly code for the strchr("'\\") bit?

> +        }
> +        break;
> +
> +    case AV_ESCAPE_MODE_QUOTE:
> +        /* enclose the string between '' */
> +        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 dstbuf->len;
> +}
> +
>  #ifdef TEST
>  
>  #undef printf
> diff --git a/libavutil/bprint.h b/libavutil/bprint.h
> index f3915fe..b9039bd 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,18 @@ 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, see AV_ESCAPE_MODE_* macros
> + * @param flags         flags which control how to escape, see AV_ESCAPE_FLAG_* macros
> + * @return the length of the buffered string so far, a negative value in case of error
> + * @see av_escape()
> + */
> +int av_bprint_escape(AVBPrint *dstbuf, const char *src, const char *special_chars,
> +                     enum AVEscapeMode mode, int flags);
> +
>  #endif /* AVUTIL_BPRINT_H */
> diff --git a/tools/ffescape.c b/tools/ffescape.c
> index d777fe4..cd2f56f 100644
> --- a/tools/ffescape.c
> +++ b/tools/ffescape.c
> @@ -42,80 +42,16 @@ static void usage(void)
>      printf("\n"
>             "Options:\n"
>             "-e                echo each input line on output\n"
> +           "-f flag           select an escape flag, can assume the values 'whitespace' and 'strict'\n"
>             "-h                print this help\n"
>             "-i INFILE         set INFILE as input file, stdin if omitted\n"
>             "-l LEVEL          set the number of escaping levels, 1 if omitted\n"
> -           "-m ESCAPE_MODE    select escape mode between 'full', 'lazy', 'quote', default is 'lazy'\n"
> +           "-m ESCAPE_MODE    select escape mode between 'backslash', 'quote', default is 'backslash'\n"
>             "-o OUTFILE        set OUTFILE as output file, stdout if omitted\n"
>             "-p PROMPT         set output prompt, is '=> ' by default\n"
>             "-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,13 +59,14 @@ 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_BACKSLASH;
> +    int escape_flags = 0;
>      int level = 1;
>      int echo = 0;
>      char *special_chars = NULL;
>      int c;
>  
> -    while ((c = getopt(argc, argv, "ehi:l:o:m:p:s:")) != -1) {
> +    while ((c = getopt(argc, argv, "ef:hi:l:o:m:p:s:")) != -1) {
>          switch (c) {
>          case 'e':
>              echo = 1;
> @@ -140,6 +77,16 @@ int main(int argc, char **argv)
>          case 'i':
>              infilename = optarg;
>              break;
> +        case 'f':
> +            if      (!strcmp(optarg, "whitespace")) escape_flags |= AV_ESCAPE_FLAG_WHITESPACE;
> +            else if (!strcmp(optarg, "strict"))     escape_flags |= AV_ESCAPE_FLAG_STRICT;
> +            else {
> +                av_log(NULL, AV_LOG_ERROR,
> +                       "Invalid value '%s' for option -f, "
> +                       "valid arguments are 'whitespace', and 'strict'\n", optarg);
> +                return 1;
> +            }
> +            break;
>          case 'l':
>          {
>              char *tail;
> @@ -154,13 +101,12 @@ 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, "backslash")) escape_mode = AV_ESCAPE_MODE_BACKSLASH;
> +            else if (!strcmp(optarg, "quote"))     escape_mode = AV_ESCAPE_MODE_QUOTE;
>              else {
>                  av_log(NULL, AV_LOG_ERROR,
>                         "Invalid value '%s' for option -m, "
> -                       "valid arguments are 'full', 'lazy', 'quote'\n", optarg);
> +                       "valid arguments are 'backslash', and 'quote'\n", optarg);
>                  return 1;
>              }
>              break;
> @@ -219,7 +165,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, escape_flags) < 0) {
>              av_log(NULL, AV_LOG_ERROR, "Could not escape string\n");
>              return 1;
>          }

No other remarks this time.

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/20130124/099636a7/attachment.asc>


More information about the ffmpeg-devel mailing list