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

Stefano Sabatini stefasab at gmail.com
Fri Jan 11 15:39:47 CET 2013


On date Thursday 2013-01-10 19:33:35 +0100, Nicolas George encoded:
> Le nonidi 19 nivôse, an CCXXI, Stefano Sabatini a écrit :
> > >From 1e67bf4479879ad45da88e005578d56952ccde33 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 |   18 ++++++++++
> >  libavutil/avstring.h |   37 +++++++++++++++++++-
> >  libavutil/bprint.c   |   45 ++++++++++++++++++++++++
> >  libavutil/bprint.h   |   15 ++++++++
> >  tools/ffescape.c     |   92 +++++++++++---------------------------------------
> >  5 files changed, 133 insertions(+), 74 deletions(-)
> > 
> > diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> > index 7072a55..35719fd 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,23 @@ 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;
> > +
> > +    av_bprint_init(&dstbuf, 1, AV_BPRINT_SIZE_UNLIMITED);
> > +
> > +    av_bprint_escape(&dstbuf, src, special_chars, mode, flags);
> 
> > +    if (!av_bprint_is_complete(&dstbuf)) {
> > +        av_bprint_finalize(&dstbuf, NULL);
> > +        return AVERROR(ENOMEM);
> > +    } else {
> > +        int len = dstbuf.len;
> > +        av_bprint_finalize(&dstbuf, dst);
> > +        return len;
> > +    }
> 

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

> Also, it will not change dstbuf.len: you do not have to
> keep a copy of it.

Changed.

> 
> > +}
> >  
> >  #ifdef TEST
> >  
> > diff --git a/libavutil/avstring.h b/libavutil/avstring.h
> > index d7af9ec..f909c12 100644
> > --- a/libavutil/avstring.h
> > +++ b/libavutil/avstring.h
> > @@ -202,7 +202,6 @@ int av_strcasecmp(const char *a, const char *b);
> >   */
> >  int av_strncasecmp(const char *a, const char *b, size_t n);
> >  
> 
> > -
> 
> Spurious but harmless.

Removed.

> >  /**
> >   * Thread safe basename.
> >   * @param path the path, on DOS both \ and / are considered separators.
> > @@ -218,6 +217,42 @@ 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);
> 

> I really think the flags and mode could be the same argument, but as you
> prefer.

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.

> 
> Also, there is no "do for the best" mode, where the function selects between
> \ and '' and nothing (and possible other future escaping methods) depending
> on the string.
> 
> > +
> >  /**
> >   * @}
> >   */
> > diff --git a/libavutil/bprint.c b/libavutil/bprint.c
> > index 4684ab4..64cd8db 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,50 @@ 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)
> > +{
> > +    switch (mode) {
> > +    case AV_ESCAPE_MODE_BACKSLASH:
> > +        /* \-escape characters */
> > +
> > +        /* escape initial whitespace */
> > +        if (!(flags & AV_ESCAPE_FLAG_STRICT) && strchr(WHITESPACES, *src))
> > +            av_bprintf(dstbuf, "\\%c", *src++);
> > +
> > +        for (; *src; src++) {
> > +            int is_last = !*(src+1);
> 
> > +            if ((is_last && !(flags & AV_ESCAPE_FLAG_STRICT) && strchr(WHITESPACES, *src)) ||
> > +                (special_chars && strchr(special_chars, *src)) ||
> > +                strchr("'\\", *src) ||
> > +                (flags & AV_ESCAPE_FLAG_WHITESPACE && strchr(WHITESPACES, *src)))
> 
> I wonder whether this big condition could be simplified by using a byte- or
> bit-array:
> 
> 	char special[256] = { 0 };
> 	for (p = special_chars; *p; p++) special[*p] = 1;
> 	if (!(flags & AV_ESCAPE_FLAG_STRICT))
> 	    special['\\'] = special['\''] = 1;
> 	if (flags & AV_ESCAPE_FLAG_WHITESPACE)
> 	    special[' '] = special['\t'] = 1;
> 
> Also, I wonder whether the last statement of the condition will not escape
> the first space twice.

Changed the logic by adding some intermediary variables to make the
logic more explicit.
 
> 
> > +                av_bprint_chars(dstbuf, '\\', 1);
> > +            av_bprint_chars(dstbuf, *src, 1);
> > +        }
> > +        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;
> 

> Why return anything? My philosophy for bprint is: add anything to it, do not
> check for errors, check for errors at the end. Having all functions void
> shows this.
> 
> The only error case here, apart from truncation inside bprint, is an invalid
> mode selected. It is a programmatic error, not an environmental one, so a
> less convenient way of reporting it would not be a problem.

Truncation error is not considered an error here, invalid parameters
yes. Also in case more complex operations are performed (e.g. alloc)
it would be handy to be able to return an error.

Updated.
-- 
FFmpeg = Free and Freak MultiPurpose Esoteric Geisha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavu-add-escape-API.patch
Type: text/x-diff
Size: 12013 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130111/8e7a3304/attachment.bin>


More information about the ffmpeg-devel mailing list