[Qemu-devel] Curl updates

[Qemu-devel] Curl updates

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

The new syntax is:

  http://user:password@example.com/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

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

[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]

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(-)

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;
-- 
1.9.0

[Qemu-devel] [PATCH 2/4] curl: Add sslverify option

[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 <mbooth@redhat.com>
---
 block/curl.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index e31b6f3..8cf0a3e 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -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");
-- 
1.9.0

[Qemu-devel] [PATCH 3/4] curl: Add usage documentation

[Matthew Booth]

Signed-off-by: Matthew Booth <mbooth@redhat.com>
---
 qemu-options.hx | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 781af14..4cc36bf 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -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
-- 
1.9.0

[Qemu-devel] [PATCH 4/4] curl: Fix build when curl_multi_socket_action isn't available

[Matthew Booth]

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

diff --git a/block/curl.c b/block/curl.c
index 8cf0a3e..e5581f5 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -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 | \
-- 
1.9.0

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

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(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] Curl updates

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]

On 05/08/2014 02:42 AM, Matthew Booth wrote:
> [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:password@example.com/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.

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

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] Curl updates

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]

Am 13.05.2014 um 21:47 hat Eric Blake geschrieben:
> On 05/08/2014 02:42 AM, Matthew Booth wrote:
> > [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:password@example.com/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?

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] Curl updates

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On 05/14/2014 01:48 AM, Kevin Wolf wrote:
> 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Matthew Booth]

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

Re: [Qemu-devel] Curl updates

[Matthew Booth]

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

On 13/05/14 15:47, Eric Blake wrote:
> On 05/08/2014 02:42 AM, Matthew Booth wrote:
>> [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:password@example.com/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.

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

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

Re: [Qemu-devel] Curl updates

[Matthew Booth]

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

On 14/05/14 03:48, Kevin Wolf wrote:
> Am 13.05.2014 um 21:47 hat Eric Blake geschrieben:
>> On 05/08/2014 02:42 AM, Matthew Booth wrote:
>>> [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:password@example.com/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?
> 
> 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-----

Re: [Qemu-devel] Curl updates

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

Am 14.05.2014 um 18:08 hat Matthew Booth geschrieben:
> On 14/05/14 03:48, Kevin Wolf wrote:
> > Am 13.05.2014 um 21:47 hat Eric Blake geschrieben:
> >> On 05/08/2014 02:42 AM, Matthew Booth wrote:
> >>> [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:password@example.com/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?
> > 
> > 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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2029 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] Curl updates

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

On 05/14/2014 10:08 AM, Matthew Booth wrote:
>> 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] Curl updates

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

On 05/14/2014 10:06 AM, Matthew Booth wrote:

>>> The new syntax is:
>>>
>>> http://user:password@example.com/path?query[opt1=val:opt2=val]
>>>

> 
> 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:password@example.com/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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] Curl updates

[Matthew Booth]

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

On 14/05/14 13:02, Eric Blake wrote:
> On 05/14/2014 10:06 AM, Matthew Booth wrote:
> 
>>>> The new syntax is:
>>>> 
>>>> http://user:password@example.com/path?query[opt1=val:opt2=val]
>>>>
>
>>>> 
>> 
>> 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:password@example.com/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-----

Re: [Qemu-devel] Curl updates

[Matthew Booth]

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

On 14/05/14 12:43, Kevin Wolf wrote:
> Am 14.05.2014 um 18:08 hat Matthew Booth geschrieben:
>> On 14/05/14 03:48, Kevin Wolf wrote:
>>> Am 13.05.2014 um 21:47 hat Eric Blake geschrieben:
>>>> On 05/08/2014 02:42 AM, Matthew Booth wrote:
>>>>> [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:password@example.com/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?
>>> 
>>> 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.
> 
> 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

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

Re: [Qemu-devel] Curl updates

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

On 05/14/2014 03:20 PM, Matthew Booth wrote:

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]