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 <mbo...@redhat.com>
> ---
> 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
