diff mbox

[1/4] curl: Fix parsing of readahead option from filename

Message ID 1399538540-5076-2-git-send-email-mbooth@redhat.com
State New
Headers show

Commit Message

Matthew Booth May 8, 2014, 8:42 a.m. UTC
curl_parse_filename wasn't removing the option string from the url,
resulting in a 404.

This change is a rewrite of the previous parsing behaviour, and
also changes the option syntax. The new syntax is:

  http://example.com/path?query[sslverify=off:readahead=64k]

The new syntax is well defined as long as square brackets in the URI
are escaped.

This change is also preparation for the addition of more options.

Signed-off-by: Matthew Booth <mbooth@redhat.com>
---
 block/curl.c | 101 +++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 67 insertions(+), 34 deletions(-)

Comments

Eric Blake May 13, 2014, 5:29 p.m. UTC | #1
On 05/08/2014 02:42 AM, Matthew Booth wrote:
> curl_parse_filename wasn't removing the option string from the url,
> resulting in a 404.
> 
> This change is a rewrite of the previous parsing behaviour, and
> also changes the option syntax. The new syntax is:
> 
>   http://example.com/path?query[sslverify=off:readahead=64k]

Again, I'm not sure I'm happy with this - we shouldn't be inventing our
own syntax when URI is already a well-defined syntax.

> 
> The new syntax is well defined as long as square brackets in the URI
> are escaped.
> 
> This change is also preparation for the addition of more options.
> 
> Signed-off-by: Matthew Booth <mbooth@redhat.com>
> ---
>  block/curl.c | 101 +++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 67 insertions(+), 34 deletions(-)
>
Matthew Booth May 14, 2014, 4 p.m. UTC | #2
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 13/05/14 13:29, Eric Blake wrote:
> On 05/08/2014 02:42 AM, Matthew Booth wrote:
>> curl_parse_filename wasn't removing the option string from the
>> url, resulting in a 404.
>> 
>> This change is a rewrite of the previous parsing behaviour, and 
>> also changes the option syntax. The new syntax is:
>> 
>> http://example.com/path?query[sslverify=off:readahead=64k]
> 
> Again, I'm not sure I'm happy with this - we shouldn't be inventing
> our own syntax when URI is already a well-defined syntax.

I don't understand. Are you suggesting adding the options as query
parameters to the URI? We obviously can't do that, because it would
change the URI. Neither can we assume that the URI will not contain
query parameters and we have them to ourselves. For example, vsphere
disk URIs, which is what I'm targetting, contain query parameters.

Matt

> 
>> 
>> The new syntax is well defined as long as square brackets in the
>> URI are escaped.
>> 
>> This change is also preparation for the addition of more
>> options.
>> 
>> Signed-off-by: Matthew Booth <mbooth@redhat.com> --- block/curl.c
>> | 101
>> +++++++++++++++++++++++++++++++++++++++-------------------- 1
>> file changed, 67 insertions(+), 34 deletions(-)
>> 
> 


- -- 
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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlNzkyAACgkQNEHqGdM8NJAdggCfXzCS0LtG+kzq5TT/ldcCX6tr
o+oAniBxwg/7yw6w8PVw6SSXtBXt4zz0
=tuPB
-----END PGP SIGNATURE-----
Eric Blake May 14, 2014, 4:55 p.m. UTC | #3
On 05/14/2014 10:00 AM, Matthew Booth wrote:
> On 13/05/14 13:29, Eric Blake wrote:
>> On 05/08/2014 02:42 AM, Matthew Booth wrote:
>>> curl_parse_filename wasn't removing the option string from the
>>> url, resulting in a 404.
>>>
>>> This change is a rewrite of the previous parsing behaviour, and 
>>> also changes the option syntax. The new syntax is:
>>>
>>> http://example.com/path?query[sslverify=off:readahead=64k]
> 
>> Again, I'm not sure I'm happy with this - we shouldn't be inventing
>> our own syntax when URI is already a well-defined syntax.
> 
> I don't understand. Are you suggesting adding the options as query
> parameters to the URI? We obviously can't do that, because it would
> change the URI. Neither can we assume that the URI will not contain
> query parameters and we have them to ourselves. For example, vsphere
> disk URIs, which is what I'm targetting, contain query parameters.

I think the better suggestion has already been made elsewhere in the
thread: _don't_ allow sslverify or readahead as parameters in the
filename, but require them to be separate parameters.  And don't add any
new locally interpreted parameters in the filename.  The filename should
be passed, intact, to the curl command, and any additional parameters
that affect how we use curl should be separate options rather than
trying to encode them into the curl filename.  We have the new json:
string parsing to make it possible to encode a single string that
includes both the curl filename URL and any separate options for
contexts where an option must be supplied but encoded into a single string.

So, a proper command line would include something like:

-drive
"file.driver=curl,file.filename=http://example.com/path?query...,file.sslverify=off,file.readahead=64k"

and the URL passed to curl is passed verbatim, without trying to pack in
any extra stuff needed locally.
diff mbox

Patch

diff --git a/block/curl.c b/block/curl.c
index d2f1084..e31b6f3 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 {
@@ -393,45 +396,74 @@  static void curl_clean_state(CURLState *s)
     s->in_use = 0;
 }
 
+/* Parse options out of @filename, which should be a URI.
+ *
+ * Options are given as a sequence of key=value pairs separated by colons and
+ * enclosed in square brackets at the end of the URI. e.g.:
+ *
+ * http://example.com/path?query[sslverify=off:readahead=64k]
+ *
+ * Consequently, if a valid URI ends with a block enclosed by square brackets,
+ * those brackets MUST be URI encoded.
+ */
 static void curl_parse_filename(const char *filename, QDict *options,
                                 Error **errp)
 {
+    char *file = g_strdup(filename);
+    char *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) {
+        /* Look for the opening bracket */
+        char *open = memrchr(file, '[', end - file);
+
+        if (NULL != open) {
+            /* NULL terminate the preceding URI by overwriting the opening
+             * bracket */
+            *open = '\0';
+            char *opt_block = open + 1;
+
+            /* NULL terminate the option string by overwriting the closing
+             * bracket */
+            *end = '\0';
+
+            /* Parse each colon-separated option */
+            char *saveptr;
+            char *opt = strtok_r(opt_block, ":", &saveptr);
+            while (NULL != opt) {
+                size_t opt_len = strlen(opt);
+
+                /* Look for an equals sign */
+                char *equals = memchr(opt, '=', opt_len);
+                if (NULL == equals) {
+                    error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt,
+                              "a value");
+                    goto out;
+                }
 
-    #define RA_OPTSTR ":readahead="
-    char *file;
-    char *ra;
-    const char *ra_val;
-    int parse_state = 0;
-
-    file = g_strdup(filename);
-
-    /* Parse a trailing ":readahead=#:" param, if present. */
-    ra = file + strlen(file) - 1;
-    while (ra >= file) {
-        if (parse_state == 0) {
-            if (*ra == ':') {
-                parse_state++;
-            } else {
-                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 key_len = equals - opt;
+                char *value = equals + 1;
+
+                if (key_len == strlen(CURL_BLOCK_OPT_READAHEAD) &&
+                    memcmp(opt, CURL_BLOCK_OPT_READAHEAD, key_len) == 0) {
+                    qdict_put(options, CURL_BLOCK_OPT_READAHEAD,
+                              qstring_from_str(value));
+                } else {
+                    *equals = '\0';
+                    error_set(errp, QERR_INVALID_PARAMETER, opt);
+                    goto out;
                 }
-                break;
+
+                opt = strtok_r(NULL, ":", &saveptr);
             }
         }
-        ra--;
     }
 
-    qdict_put(options, "url", qstring_from_str(file));
+    qdict_put(options, CURL_BLOCK_OPT_URL, qstring_from_str(file));
 
+out:
     g_free(file);
 }
 
@@ -440,12 +472,12 @@  static QemuOptsList runtime_opts = {
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
         {
-            .name = "url",
+            .name = CURL_BLOCK_OPT_URL,
             .type = QEMU_OPT_STRING,
             .help = "URL to open",
         },
         {
-            .name = "readahead",
+            .name = CURL_BLOCK_OPT_READAHEAD,
             .type = QEMU_OPT_SIZE,
             .help = "Readahead size",
         },
@@ -477,14 +509,15 @@  static int curl_open(BlockDriverState *bs, QDict *options, int flags,
         goto out_noclean;
     }
 
-    s->readahead_size = qemu_opt_get_size(opts, "readahead", READ_AHEAD_SIZE);
+    s->readahead_size = qemu_opt_get_size(opts, CURL_BLOCK_OPT_READAHEAD,
+                                          READ_AHEAD_DEFAULT);
     if ((s->readahead_size & 0x1ff) != 0) {
         error_setg(errp, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512",
                    s->readahead_size);
         goto out_noclean;
     }
 
-    file = qemu_opt_get(opts, "url");
+    file = qemu_opt_get(opts, CURL_BLOCK_OPT_URL);
     if (file == NULL) {
         error_setg(errp, "curl block driver requires an 'url' option");
         goto out_noclean;