[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