qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/3] curl: Fix parsing of readahead option from


From: Kevin Wolf
Subject: Re: [Qemu-devel] [PATCH 1/3] curl: Fix parsing of readahead option from filename
Date: Wed, 30 Apr 2014 17:16:38 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

Am 30.04.2014 um 16:20 hat Matthew Booth geschrieben:
> curl_parse_filename wasn't removing the option string from the url,
> resulting in a 404.
> 
> This change is a essentially a rewrite of that function as I also need
> to extend it to handle more options. The rewrite is also much easier
> to read.
> 
> Signed-off-by: Matthew Booth <address@hidden>
> ---
>  block/curl.c | 81 
> ++++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 52 insertions(+), 29 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index d2f1084..4de6856 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -46,12 +46,15 @@
>  #define CURL_NUM_STATES 8
>  #define CURL_NUM_ACB    8
>  #define SECTOR_SIZE     512
> -#define READ_AHEAD_SIZE (256 * 1024)
> +#define READ_AHEAD_DEFAULT (256 * 1024)
>  
>  #define FIND_RET_NONE   0
>  #define FIND_RET_OK     1
>  #define FIND_RET_WAIT   2
>  
> +#define CURL_BLOCK_OPT_URL       "url"
> +#define CURL_BLOCK_OPT_READAHEAD "readahead"
> +
>  struct BDRVCURLState;
>  
>  typedef struct CURLAIOCB {
> @@ -396,41 +399,60 @@ static void curl_clean_state(CURLState *s)
>  static void curl_parse_filename(const char *filename, QDict *options,
>                                  Error **errp)
>  {
> -
> -    #define RA_OPTSTR ":readahead="
>      char *file;
> -    char *ra;
> -    const char *ra_val;
> -    int parse_state = 0;
> +    char *end;
>  
>      file = g_strdup(filename);
> +    end = file + strlen(file) - 1;
> +
> +    /* Don't fail if we've been passed an empty string.
> +     * Only examine strings with a trailing ':'
> +     */
> +    if (end >= file && *end == ':') {
> +        /* We exit this loop when we don't find a recognised option 
> immediately
> +         * preceding the trailing ':' of the form ':<option>=<value>'
> +         *
> +         * If filename has a trailing ':' but no preceding options, we don't
> +         * remove the trailing ':'.
> +         */
> +        for (;;) {
> +            /* Look for the preceding colon */
> +            char *colon = memrchr(file, ':', end - file);
> +            if (NULL == colon) {
> +                break;
> +            }
>  
> -    /* Parse a trailing ":readahead=#:" param, if present. */
> -    ra = file + strlen(file) - 1;
> -    while (ra >= file) {
> -        if (parse_state == 0) {
> -            if (*ra == ':') {
> -                parse_state++;
> -            } else {
> +            char *opt_start = colon + 1;
> +
> +            /* Look for an equals sign */
> +            char *equals = memchr(opt_start, '=', end - opt_start);
> +            if (NULL == equals) {
>                  break;
>              }
> -        } else if (parse_state == 1) {
> -            if (*ra > '9' || *ra < '0') {
> -                char *opt_start = ra - strlen(RA_OPTSTR) + 1;
> -                if (opt_start > file &&
> -                    strncmp(opt_start, RA_OPTSTR, strlen(RA_OPTSTR)) == 0) {
> -                    ra_val = ra + 1;
> -                    ra -= strlen(RA_OPTSTR) - 1;
> -                    *ra = '\0';
> -                    qdict_put(options, "readahead", 
> qstring_from_str(ra_val));
> -                }
> +
> +            size_t opt_len = equals - opt_start;
> +            char *value = equals + 1;
> +            size_t value_len = end - value;
> +
> +            if (opt_len == strlen(CURL_BLOCK_OPT_READAHEAD) &&
> +                memcmp(opt_start, CURL_BLOCK_OPT_READAHEAD, opt_len) == 0) {
> +                /* This is redundant after the first iteration */
> +                *end = '\0';
> +                qdict_put(options, CURL_BLOCK_OPT_READAHEAD,
> +                          qstring_from_str(value));
> +            } else {
> +                /* Unknown option */
>                  break;

So if we get an unknown option, we simply abort parsing the filename.
This means that we'll try to open a URL that still contains an option
and will probably fail with a rather confusing error message.

Shouldn't we set a clear error message about the unknown option here
with error_setg() and immediately return?

Rest looks okay.

Kevin



reply via email to

[Prev in Thread] Current Thread [Next in Thread]