From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:48893)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mbooth@redhat.com>) id 1WfmnO-0001xI-Ms
	for qemu-devel@nongnu.org; Thu, 01 May 2014 04:56:52 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mbooth@redhat.com>) id 1WfmnI-00066A-Hd
	for qemu-devel@nongnu.org; Thu, 01 May 2014 04:56:46 -0400
Received: from mx1.redhat.com ([209.132.183.28]:38293)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mbooth@redhat.com>) id 1WfmnI-00065G-9e
	for qemu-devel@nongnu.org; Thu, 01 May 2014 04:56:40 -0400
Message-ID: <53620C41.3040506@redhat.com>
Date: Thu, 01 May 2014 09:56:33 +0100
From: Matthew Booth <mbooth@redhat.com>
MIME-Version: 1.0
References: <1398867658-2069-1-git-send-email-mbooth@redhat.com>
	<1398867658-2069-2-git-send-email-mbooth@redhat.com>
	<20140430151638.GE4380@noname.redhat.com>
In-Reply-To: <20140430151638.GE4380@noname.redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 1/3] curl: Fix parsing of readahead option
	from filename
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Kevin Wolf <kwolf@redhat.com>
Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, stefanha@redhat.com

On 30/04/14 16:16, Kevin Wolf wrote:
> 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 <mbooth@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?

TBH I was just trying to stay compatible with the behaviour which most
recently worked. Hence the weirdness with the trailing ':', for example.

I did consider whether to do ignore these options or not. I decided to
ignore them because the option syntax isn't safe: the string
':readahead=1k:' could be found at the end of a valid URL. Ignoring
unknown options reduces the chances of a false positive. For example, in:

http://example.com/path?query=foo:

How do you avoid throwing an error about the unknown option
'//example.com/path?query'? I could probably chuck in a couple of
heuristics to avoid the obvious false positives, but it would be even
messier. The best option is not to do this at all, and use the separated
options command line syntax. In fact, I might add a note to the docs
advising this.

Alternatively I could completely change the syntax. However, the only
'safe' syntax I can think of would involve ugly custom escaping, which
would probably catch out more people than unsafe option parsing. I'm
open to suggestions.

On a related note, do you know if it's possible to specify a backing
file with separated options? i.e.:

qemu-img create -f qcow2 -o
backingfile.url='http://example.com/path',backingfile.readahead='64k'
/tmp/foo.qcow2

I suspect that qcow2 only stores a string, so probably not, but I
thought I'd ask.

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490