Skip to contentSkip to conversation

Curl updates

Qemu-devel In review Matthew Booth 3 participants 19 messages May 8, 2014 – May 14, 2014 Block layer

lore
Share & tools
lore

Matthew Booth

author
#0 lore raw

[PATCH 1/4] curl: Fix parsing of readahead option from filename [PATCH 2/4] curl: Add sslverify option
[PATCH 3/4] curl: Add usage documentation

The first 3 patches are reposted with updates following discussion of the option syntax. With this patch I've decided to break entirely with the previous syntax. Given that option parsing was previously both broken and undocumented, this is hopefully a forgivable sin.

The new syntax is:

http://user:[email protected]/path?query[opt1=val:opt2=val]

I've bounded the option block in square brackets as these have no semantic meaning in any of the supported URI formats. Consequently the user can escape them if they're unfortunate enough to have a URI which looks like it contains an option block.

I decided to separate options with colons rather than commas because commas play havoc with qemu's command line parsing. There's presumably a way round this, but I couldn't guess it and I was too lazy to look it up, so I assume users would feel the same.

As options are now unambigous, invalid options now result in an error.

[PATCH 4/4] curl: Fix build when curl_multi_socket_action isn't

The last patch is unrelated. It should fix build against old curl, although I don't have an old curl kicking around to test it against.

I still have a couple of patches in my local tree which:

* Remove blocking behaviour in curl_open
* Don't send EIO when a read connection hits a timeout

I'm also planning to add another option for timeout length, and to implement write support.

Matt

author
#1 lore raw

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

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 <[email protected]>
---
 block/curl.c | 101 +++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 67 insertions(+), 34 deletions(-)
block/curl.c +67 -34
index d2f1084..e31b6f3 100644
@@ -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;
Signature · 1 line
1.9.0
#2 lore raw

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

Quoted 7 lines · Matthew Booth

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.

Quoted 11 lines

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 <[email protected]>
---
 block/curl.c | 101 +++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 67 insertions(+), 34 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
author
#3 lore raw

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 11 lines · Eric Blake
Quoted 7 lines · Matthew Booth

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

Quoted 5 lines
Quoted 5 lines

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

Quoted 9 lines
Quoted 8 lines

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

Signed-off-by Matthew Booth <[email protected]> --- 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-----

#4 lore raw

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

Quoted 10 lines · Matthew Booth
Quoted 8 lines · Eric Blake
Quoted 7 lines · Matthew Booth

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]

Quoted 8 lines
Quoted 2 lines

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.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
author
#5 lore raw

PATCH 2/4 curl: Add sslverify option

In reply to Matthew Booth

This allows qemu to use images over https with a self-signed certificate. It defaults to verifying the certificate.

Signed-off-by Matthew Booth <[email protected]>
---
 block/curl.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
block/curl.c +31 -0
index e31b6f3..8cf0a3e 100644
@@ -23,6 +23,7 @@
  */
 #include "qemu-common.h"
 #include "block/block_int.h"
+#include "qapi/qmp/qbool.h"
 #include <curl/curl.h>
 
 // #define DEBUG

@@ -54,6 +55,7 @@
 
 #define CURL_BLOCK_OPT_URL       "url"
 #define CURL_BLOCK_OPT_READAHEAD "readahead"
+#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
 
 struct BDRVCURLState;
 

@@ -91,6 +93,7 @@ typedef struct BDRVCURLState {
     CURLState states[CURL_NUM_STATES];
     char *url;
     size_t readahead_size;
+    bool sslverify;
     bool accept_range;
 } BDRVCURLState;
 

@@ -357,6 +360,7 @@ static CURLState *curl_init_state(BDRVCURLState *s)
             return NULL;
         }
         curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
+        curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER, s->sslverify);
         curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, 5);
         curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
                          (void *)curl_read_cb);

@@ -450,6 +454,26 @@ static void curl_parse_filename(const char *filename, QDict *options,
                     memcmp(opt, CURL_BLOCK_OPT_READAHEAD, key_len) == 0) {
                     qdict_put(options, CURL_BLOCK_OPT_READAHEAD,
                               qstring_from_str(value));
+                } else if (key_len == strlen(CURL_BLOCK_OPT_SSLVERIFY) &&
+                           memcmp(opt, CURL_BLOCK_OPT_SSLVERIFY,
+                                  key_len) == 0) {
+                    size_t value_len = opt_len - (value - opt);
+
+                    int sslverify;
+                    if (value_len == strlen("on") &&
+                        memcmp(value, "on", value_len) == 0) {
+                        sslverify = 1;
+                    } else if (value_len == strlen("off") &&
+                               memcmp(value, "off", value_len) == 0) {
+                        sslverify = 0;
+                    } else {
+                        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                                  CURL_BLOCK_OPT_SSLVERIFY, "'on' or 'off'");
+                        goto out;
+                    }
+
+                    qdict_put(options, CURL_BLOCK_OPT_SSLVERIFY,
+                              qbool_from_int(sslverify));
                 } else {
                     *equals = '\0';
                     error_set(errp, QERR_INVALID_PARAMETER, opt);

@@ -481,6 +505,11 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_SIZE,
             .help = "Readahead size",
         },
+        {
+            .name = CURL_BLOCK_OPT_SSLVERIFY,
+            .type = QEMU_OPT_BOOL,
+            .help = "Verify SSL certificate"
+        },
         { /* end of list */ }
     },
 };

@@ -517,6 +546,8 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
         goto out_noclean;
     }
 
+    s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
+
     file = qemu_opt_get(opts, CURL_BLOCK_OPT_URL);
     if (file == NULL) {
         error_setg(errp, "curl block driver requires an 'url' option");
Signature · 1 line
1.9.0
author
#6 lore raw

PATCH 3/4 curl: Add usage documentation

In reply to Matthew Booth
Signed-off-by Matthew Booth <[email protected]>
---
 qemu-options.hx | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
qemu-options.hx +72 -0
index 781af14..4cc36bf 100644
@@ -2191,6 +2191,78 @@ qemu-system-x86_64 --drive file=gluster://192.0.2.1/testvol/a.img
 @end example
 
 See also @url{http://www.gluster.org}.
+
+@item HTTP/HTTPS/FTP/FTPS/TFTP
+QEMU supports read-only access to files accessed over http(s), ftp(s) and tftp.
+
+Syntax using a single filename:
+@example
+<protocol>://[<username>[:<password>]@@]<host>/<path>['['<option>=<value>:<option>=<value>...']']
+@end example
+
+where:
+@table @option
+@item protocol
+'http', 'https', 'ftp', 'ftps', or 'tftp'.
+
+@item username
+Optional username for authentication to the remote server.
+
+@item password
+Optional password for authentication to the remote server.
+
+@item host
+Address of the remote server.
+
+@item path
+Path on the remote server, including any query string.
+@end table
+
+and options are:
+@table @option
+@item readahead
+The amount of data to read ahead with each range request to the remote server.
+This value may optionally have the suffix 'T', 'G', 'M', 'K', 'k' or 'b'. If it
+does not have a suffix, it will be assumed to be in bytes. The value must be a
+multiple of 512 bytes. It defaults to 256k.
+
+@item sslverify
+Whether to verify the remote server's certificate when connecting over SSL. It
+can have the value 'on' or 'off'. It defaults to 'on'.
+@end table
+
+Note that options are enclosed in square brackets. Consequently, if the remote
+URI ends with a block enclosed in square brackets, the square brackets must be
+URI escaped. Otherwise, the block will be interpreted as a set of options.
+
+When passing options to qemu explicitly, @option{driver} is the value of
+<protocol>, the entire URL, i.e.
+<protocol>://[<username>[:<password>]@@]<host>/<path>, is passed in
+@option{url}, and @option{readahead} and @option{sslverify} are passed
+separately with values as described above.
+
+Example: boot from a remote Fedora 20 live ISO image
+@example
+qemu-system-x86_64 --drive media=cdrom,file=http://dl.fedoraproject.org/pub/fedora/linux/releases/20/Live/x86_64/Fedora-Live-Desktop-x86_64-20-1.iso,readonly
+
+qemu-system-x86_64 --drive media=cdrom,file.driver=http,file.url=http://dl.fedoraproject.org/pub/fedora/linux/releases/20/Live/x86_64/Fedora-Live-Desktop-x86_64-20-1.iso,readonly
+@end example
+
+Example: boot from a remote Fedora 20 cloud image using a local overlay for
+writes, copy-on-read, and a readahead of 64k
+@example
+qemu-img create -f qcow2 -o backing_file='https://dl.fedoraproject.org/pub/fedora/linux/releases/20/Images/x86_64/Fedora-x86_64-20-20131211.1-sda.qcow2[readahead=64k]' /tmp/Fedora-x86_64-20-20131211.1-sda.qcow2
+
+qemu-system-x86_64 -drive file=/tmp/Fedora-x86_64-20-20131211.1-sda.qcow2,copy-on-read=on
+@end example
+
+Example: boot from an image stored on a VMware vSphere server with a self-signed
+certificate using a local overlay for writes and a readahead of 64k
+@example
+qemu-img create -f qcow2 -o backing_file='https://user:password@@vsphere.example.com/folder/test/test-flat.vmdk?dcPath=Datacenter&dsName=datastore1[sslverify=off:readahead=64k]' /tmp/test.qcow2
+
+qemu-system-x86_64 -drive file=/tmp/test.qcow2
+@end example
 ETEXI
 
 STEXI
Signature · 1 line
1.9.0
author
#7 lore raw

PATCH 4/4 curl: Fix build when curl_multi_socket_action isn't available

In reply to Matthew Booth
Signed-off-by Matthew Booth <[email protected]>
---
 block/curl.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
block/curl.c +15 -0
index 8cf0a3e..e5581f5 100644
@@ -38,6 +38,21 @@
 #if LIBCURL_VERSION_NUM >= 0x071000
 /* The multi interface timer callback was introduced in 7.16.0 */
 #define NEED_CURL_TIMER_CALLBACK
+#define HAVE_SOCKET_ACTION
+#endif
+
+#ifndef HAVE_SOCKET_ACTION
+/* If curl_multi_socket_action isn't available, define it statically here in
+ * terms of curl_multi_socket. Note that ev_bitmask will be ignored, which is
+ * less efficient but still safe. */
+static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
+                                            curl_socket_t sockfd,
+                                            int ev_bitmask,
+                                            int *running_handles)
+{
+    return curl_multi_socket(multi_handle, sockfd, running_handles);
+}
+#define curl_multi_socket_action __curl_multi_socket_action
 #endif
 
 #define PROTOCOLS (CURLPROTO_HTTP | CURLPROTO_HTTPS | \
Signature · 1 line
1.9.0
#8 lore raw
In reply to Matthew Booth
Quoted 9 lines · Matthew Booth

[PATCH 1/4] curl: Fix parsing of readahead option from filename [PATCH 2/4] curl: Add sslverify option
[PATCH 3/4] curl: Add usage documentation

The first 3 patches are reposted with updates following discussion of the option syntax. With this patch I've decided to break entirely with the previous syntax. Given that option parsing was previously both broken and undocumented, this is hopefully a forgivable sin.

Quoted 6 lines

The new syntax is:

http://user:[email protected]/path?query[opt1=val:opt2=val]

I've bounded the option block in square brackets as these have no semantic meaning in any of the supported URI formats.

Offhand, I'm not liking this. Why not use a completely valid URI, with '.../path?query&opt1=val&opt2=val'? Inventing your own [opt1=val:opt2=val] on top of URI is asking for confusion.

Are you trying to support a way to pass a query string to the curl URI, in addition to local options? How often do curl URIs need a query? Is it something where you could use a local option named '.../path?query=foo=bar' that contains anything to pass on to the raw uri for curl as '.../path?foo=bar' (that is, ALL query name=value pairs are local, but you have a name of 'query' whose value can be the URI-encoded string to pass on as the name=value pairs for the raw URI that you are passing through)? That would be more consistent so that the option is an actual URI to begin with.

Quoted 8 lines

Consequently the user can escape
them if they're unfortunate enough to have a URI which looks like it contains an option block.

I decided to separate options with colons rather than commas because commas play havoc with qemu's command line parsing. There's presumably a way round this, but I couldn't guess it and I was too lazy to look it up, so I assume users would feel the same.

Using ',,' behaves as an escape for any literal comma, when doing qemu command line option parsing.

Quoted 20 lines

As options are now unambigous, invalid options now result in an error.

[PATCH 4/4] curl: Fix build when curl_multi_socket_action isn't

The last patch is unrelated. It should fix build against old curl, although I don't have an old curl kicking around to test it against.

I still have a couple of patches in my local tree which:

* Remove blocking behaviour in curl_open
* Don't send EIO when a read connection hits a timeout

I'm also planning to add another option for timeout length, and to implement write support.

Matt

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#9 lore raw

Am 13.05.2014 um 21:47 hat Eric Blake geschrieben:

Quoted 17 lines · Matthew Booth
Quoted 9 lines

[PATCH 1/4] curl: Fix parsing of readahead option from filename [PATCH 2/4] curl: Add sslverify option
[PATCH 3/4] curl: Add usage documentation

The first 3 patches are reposted with updates following discussion of the option syntax. With this patch I've decided to break entirely with the previous syntax. Given that option parsing was previously both broken and undocumented, this is hopefully a forgivable sin.

Quoted 6 lines

The new syntax is:

http://user:[email protected]/path?query[opt1=val:opt2=val]

I've bounded the option block in square brackets as these have no semantic meaning in any of the supported URI formats.

Quoted 6 lines

Offhand, I'm not liking this. Why not use a completely valid URI, with '.../path?query&opt1=val&opt2=val'? Inventing your own [opt1=val:opt2=val] on top of URI is asking for confusion.

Are you trying to support a way to pass a query string to the curl URI, in addition to local options? How often do curl URIs need a query?

My guess would be that you need this more often than local options.

Anyway, let's not add new options encoded in the URL, but point users to separate options. We may decide that we need the support the old crude way of encoding local options for compatibility, but preferably I would make filename just a plain URL.

Kevin

#10 lore raw
Quoted 4 lines · Kevin Wolf

Anyway, let's not add new options encoded in the URL, but point users to separate options. We may decide that we need the support the old crude way of encoding local options for compatibility, but preferably I would make filename just a plain URL.

Indeed, separating options from the filename URL is ideal (don't cram more into the filename than what you actually plan to pass to curl). Since we've added support for multiple options on the command line beyond everything encoded in a filename, we should use that support.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
author
#11 lore raw
In reply to Kevin Wolf

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 27 lines · Kevin Wolf

Am 13.05.2014 um 21:47 hat Eric Blake geschrieben:

Quoted 18 lines · Matthew Booth
Quoted 10 lines

[PATCH 1/4] curl: Fix parsing of readahead option from filename [PATCH 2/4] curl: Add sslverify option [PATCH 3/4]
curl: Add usage documentation

The first 3 patches are reposted with updates following discussion of the option syntax. With this patch I've decided to break entirely with the previous syntax. Given that option parsing was previously both broken and undocumented, this is hopefully a forgivable sin.

Quoted 6 lines

The new syntax is:

http://user:[email protected]/path?query[opt1=val:opt2=val]

I've bounded the option block in square brackets as these have no semantic meaning in any of the supported URI formats.

Quoted 7 lines

Offhand, I'm not liking this. Why not use a completely valid URI, with '.../path?query&opt1=val&opt2=val'? Inventing your own [opt1=val:opt2=val] on top of URI is asking for confusion.

Are you trying to support a way to pass a query string to the curl URI, in addition to local options? How often do curl URIs need a query?

Quoted 7 lines

My guess would be that you need this more often than local options.

Anyway, let's not add new options encoded in the URL, but point users to separate options. We may decide that we need the support the old crude way of encoding local options for compatibility, but preferably I would make filename just a plain URL.

Agree, but only when we support giving options to a backing file.

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

iEYEARECAAYFAlNzlRsACgkQNEHqGdM8NJD2gQCfcjTTNukAHagZmZ8YIl7NrKxa n+YAoIju2DbERjV55zc5xt1TtLwOaq6u
=0n8Q
-----END PGP SIGNATURE-----

#12 lore raw

Am 14.05.2014 um 18:08 hat Matthew Booth geschrieben:

Quoted 31 lines · Kevin Wolf
Quoted 30 lines

Am 13.05.2014 um 21:47 hat Eric Blake geschrieben:

Quoted 18 lines · Matthew Booth
Quoted 10 lines

[PATCH 1/4] curl: Fix parsing of readahead option from filename [PATCH 2/4] curl: Add sslverify option [PATCH 3/4]
curl: Add usage documentation

The first 3 patches are reposted with updates following discussion of the option syntax. With this patch I've decided to break entirely with the previous syntax. Given that option parsing was previously both broken and undocumented, this is hopefully a forgivable sin.

Quoted 6 lines

The new syntax is:

http://user:[email protected]/path?query[opt1=val:opt2=val]

I've bounded the option block in square brackets as these have no semantic meaning in any of the supported URI formats.

Quoted 7 lines

Offhand, I'm not liking this. Why not use a completely valid URI, with '.../path?query&opt1=val&opt2=val'? Inventing your own [opt1=val:opt2=val] on top of URI is asking for confusion.

Are you trying to support a way to pass a query string to the curl URI, in addition to local options? How often do curl URIs need a query?

My guess would be that you need this more often than local options.

Quoted 6 lines
Quoted 4 lines

Anyway, let's not add new options encoded in the URL, but point users to separate options. We may decide that we need the support the old crude way of encoding local options for compatibility, but preferably I would make filename just a plain URL.

Agree, but only when we support giving options to a backing file.

Right, but we want that anyway. I applied Max's patches for the json:
pseudo-protocol today, so we should have everything we need.

Kevin

author
#13 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 20 lines · Kevin Wolf

Am 14.05.2014 um 18:08 hat Matthew Booth geschrieben:

Quoted 19 lines · Kevin Wolf
Quoted 18 lines

Am 13.05.2014 um 21:47 hat Eric Blake geschrieben:

Quoted 17 lines · Matthew Booth
Quoted 16 lines

[PATCH 1/4] curl: Fix parsing of readahead option from filename [PATCH 2/4] curl: Add sslverify option [PATCH 3/4] curl: Add usage documentation

The first 3 patches are reposted with updates following discussion of the option syntax. With this patch I've decided to break entirely with the previous syntax. Given that option parsing was previously both broken and undocumented, this is hopefully a forgivable sin.

The new syntax is:

http://user:[email protected]/path?query[opt1=val:opt2=val]

I've bounded the option block in square brackets as these have

Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Quoted 10 lines
Quoted 1 line

no semantic meaning in any of the supported URI formats.

Offhand, I'm not liking this. Why not use a completely valid URI, with '.../path?query&opt1=val&opt2=val'? Inventing your own [opt1=val:opt2=val] on top of URI is asking for confusion.

Are you trying to support a way to pass a query string to the curl URI, in addition to local options? How often do curl URIs need a query?

My guess would be that you need this more often than local options.

Anyway, let's not add new options encoded in the URL, but point users to separate options. We may decide that we need the support the old crude way of encoding local options for compatibility, but preferably I would make filename just a plain URL.

Quoted 5 lines
Quoted 2 lines

Agree, but only when we support giving options to a backing file.

Right, but we want that anyway. I applied Max's patches for the
json: pseudo-protocol today, so we should have everything we need.

I can't see this in block/master. Am I looking in the wrong place?

Matt

Quoted 3 lines

Kevin

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

iEYEARECAAYFAlNz3jQACgkQNEHqGdM8NJBBNACeKipCLadTv1+AANVJzLrMcJmU q0IAnREbaaT3LVXaIYr09eej04mzvokG
=bEUQ
-----END PGP SIGNATURE-----

#14 lore raw
Quoted 5 lines · Matthew Booth
Quoted 3 lines

Right, but we want that anyway. I applied Max's patches for the
json: pseudo-protocol today, so we should have everything we need.

I can't see this in block/master. Am I looking in the wrong place?

git://repo.or.cz/qemu/kevin.git block

or view in your browser here:
http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/block

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#15 lore raw
In reply to Matthew Booth
Quoted 6 lines · Matthew Booth
Quoted 4 lines

Anyway, let's not add new options encoded in the URL, but point users to separate options. We may decide that we need the support the old crude way of encoding local options for compatibility, but preferably I would make filename just a plain URL.

Agree, but only when we support giving options to a backing file.

Giving options as a single string in a backing file will be done via the new json: pseudo-protocol, NOT by munging the filename of the curl:
protocol.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
author
#16 lore raw
In reply to Eric Blake

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 22 lines · Eric Blake
Quoted 10 lines · Matthew Booth

[PATCH 1/4] curl: Fix parsing of readahead option from filename [PATCH 2/4] curl: Add sslverify option [PATCH 3/4] curl: Add usage documentation

The first 3 patches are reposted with updates following discussion of the option syntax. With this patch I've decided to break entirely with the previous syntax. Given that option parsing was previously both broken and undocumented, this is hopefully a forgivable sin.

Quoted 6 lines

The new syntax is:

http://user:[email protected]/path?query[opt1=val:opt2=val]

I've bounded the option block in square brackets as these have no semantic meaning in any of the supported URI formats.

Offhand, I'm not liking this. Why not use a completely valid URI, with '.../path?query&opt1=val&opt2=val'? Inventing your own [opt1=val:opt2=val] on top of URI is asking for confusion.

Quoted 9 lines

Are you trying to support a way to pass a query string to the curl URI, in addition to local options? How often do curl URIs need a query? Is it something where you could use a local option named '.../path?query=foo=bar' that contains anything to pass on to the raw uri for curl as '.../path?foo=bar' (that is, ALL query name=value pairs are local, but you have a name of 'query' whose value can be the URI-encoded string to pass on as the name=value pairs for the raw URI that you are passing through)? That would be more consistent so that the option is an actual URI to begin with.

The curse of replying without reading all outstanding responses :) I think I answered this in my previous reply.

A URI can, by definition, contain a query string, and we cannot assume that it won't. In fact, the use case I'm specifically interested in always includes a query string. If we try to overload the query string, we're adding heuristic fuzziness. My syntax makes the option string distinct from the URI, so no heuristics are required. It's also very clear to read IMHO.

Matt

Quoted 5 lines
Quoted 5 lines

Consequently the user can escape them if they're unfortunate enough to have a URI which looks like it contains an option block.

Quoted 32 lines
Quoted 5 lines

I decided to separate options with colons rather than commas because commas play havoc with qemu's command line parsing. There's presumably a way round this, but I couldn't guess it and I was too lazy to look it up, so I assume users would feel the same.

Using ',,' behaves as an escape for any literal comma, when doing qemu command line option parsing.

Quoted 22 lines

As options are now unambigous, invalid options now result in an error.

[PATCH 4/4] curl: Fix build when curl_multi_socket_action isn't

The last patch is unrelated. It should fix build against old curl, although I don't have an old curl kicking around to test it against.

I still have a couple of patches in my local tree which:

* Remove blocking behaviour in curl_open * Don't send EIO when a read connection hits a timeout

I'm also planning to add another option for timeout length, and to implement write support.

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

iEYEARECAAYFAlNzlHUACgkQNEHqGdM8NJAJBgCeK8+XmnVN/p0DFKyhiM0A/+RD 6yQAoJqUfJ6JrGhKT7Nv8HhLAeVstxyz
=91Eb
-----END PGP SIGNATURE-----

#17 lore raw
Quoted 6 lines · Matthew Booth
Quoted 4 lines
Quoted 4 lines

The new syntax is:

http://user:[email protected]/path?query[opt1=val:opt2=val]

Quoted 6 lines

A URI can, by definition, contain a query string, and we cannot assume that it won't. In fact, the use case I'm specifically interested in always includes a query string. If we try to overload the query string, we're adding heuristic fuzziness. My syntax makes the option string distinct from the URI, so no heuristics are required. It's also very clear to read IMHO.

But your proposed syntax is no longer a URI. I'd much rather see:

'json:{"driver":"curl","filename":"http://user:[email protected]/path?query","opt1":"val","opt2":"val"}'

which then shares the same syntax as all other drivers for creating a flat string that encodes multiple pieces of information, rather than having to overload the filename to be a non-URI encoding locally useful information.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
author
#18 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 16 lines · Eric Blake
Quoted 7 lines · Matthew Booth
Quoted 6 lines
Quoted 6 lines

The new syntax is:

http://user:[email protected]/path?query[opt1=val:opt2=val]

Quoted 6 lines

A URI can, by definition, contain a query string, and we cannot assume that it won't. In fact, the use case I'm specifically interested in always includes a query string. If we try to overload the query string, we're adding heuristic fuzziness. My syntax makes the option string distinct from the URI, so no heuristics are required. It's also very clear to read IMHO.

Quoted 9 lines

But your proposed syntax is no longer a URI. I'd much rather see:

'json:{"driver":"curl","filename":"http://user:[email protected]/path?query","opt1":"val","opt2":"val"}'

which then shares the same syntax as all other drivers for
creating a flat string that encodes multiple pieces of information, rather than having to overload the filename to be a non-URI encoding locally useful information.

Agree: if it's possible to pass explicit parameters to a backing file then we should ditch the hokey parsing. I'll check out the new syntax, rip out the parser and update the docs.

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

iEYEARECAAYFAlNz1f4ACgkQNEHqGdM8NJBQ8ACfeekpMvSJS0kh1sx+/gtT6lS6 nwgAn2yxW2ympFXfvTybxQhBfdL907Cc
=HDQu
-----END PGP SIGNATURE-----