From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:53092)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1WhFBV-0005X3-FD
	for qemu-devel@nongnu.org; Mon, 05 May 2014 05:27:47 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1WhFBP-0000pU-8V
	for qemu-devel@nongnu.org; Mon, 05 May 2014 05:27:41 -0400
Received: from mx1.redhat.com ([209.132.183.28]:9619)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1WhFBP-0000pQ-17
	for qemu-devel@nongnu.org; Mon, 05 May 2014 05:27:35 -0400
Date: Mon, 5 May 2014 11:27:30 +0200
From: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20140505092730.GC3317@noname.str.redhat.com>
References: <1398867658-2069-1-git-send-email-mbooth@redhat.com>
	<1398867658-2069-2-git-send-email-mbooth@redhat.com>
	<20140430151638.GE4380@noname.redhat.com>
	<53620C41.3040506@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <53620C41.3040506@redhat.com>
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: Matthew Booth <mbooth@redhat.com>
Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, stefanha@redhat.com

Am 01.05.2014 um 10:56 hat Matthew Booth geschrieben:
> 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'?

You don't? :-)

It's the same thing for local file names. If you want a safe way for
specifying files that have things like colons in their name, you
shouldn't use the filename string, but specific options that are taken
literally (like file.filename=foo:bar.qcow2), otherwise you get an error
about unknown protocols.

And Eric made the very good point that colons should be percent-encoded
anyway in a URL, so I really wouldn't worry too much about this. There
are enough ways for the user to achieve what they want, we don't have
to provide everything in the shortcut string.

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

Let's not go there. We already have a safe syntax, and it's the
separated options.

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

Like Eric said, the json: protocol is how we want to implement this.

If you're okay with specifying the option at runtime, you can already do
that with an option like this:

    -drive file=/tmp/foo.qcow2,backing.file.readahead=64k

Kevin