[Qemu-devel] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema

[Qemu-devel] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema

[Max Reitz]

Yes, it's yet another episode in our popular
get-blockdev-add-ready-for-2.9 drama!

Right now, the schema for the curl block driver is seriously lacking.
This series improves things at least a bit.

To improve things seriously, we might want to structure the URL instead
of it being just a plain string, and we might want to split the cookie
string into a list of dicts or something similar. However, strictly
speaking our curl block driver is *not* an (ht|f)tps? block driver but
just a curl driver. All it does is pass some options to libcurl and then
send and receive data from it. (We really should have just named it
"curl" from the start.)

Therefore, it probably is for the best to leave these options rather
opaque and let libcurl do the interpretation.


Max Reitz (2):
  qapi/curl: Extend and fix blockdev-add schema
  block/curl: Check protocol prefix

 qapi/block-core.json | 103 ++++++++++++++++++++++++++++++++++++++++++++++-----
 block/curl.c         |  10 +++++
 2 files changed, 104 insertions(+), 9 deletions(-)

-- 
2.12.1

[Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: Extend and fix blockdev-add schema

[Max Reitz]

The curl block driver accepts more options than just "filename"; also,
the URL is actually expected to be passed through the "url" option
instead of "filename".

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json | 103 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 94 insertions(+), 9 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index b5f0e9958c..033457ce86 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2737,16 +2737,101 @@
             '*debug': 'int' } }
 
 ##
-# @BlockdevOptionsCurl:
+# @BlockdevOptionsCurlBase:
 #
-# Driver specific block device options for the curl backend.
+# Driver specific block device options shared by all protocols supported by the
+# curl backend.
 #
-# @filename:    path to the image file
+# @url:                     URL of the image file
+#
+# @readahead:               Size of the read-ahead cache; must be a multiple of
+#                           512 (defaults to 256 kB)
+#
+# @timeout:                 Timeout for connections, in seconds (defaults to 5)
+#
+# @username:                Username for authentication (defaults to none)
+#
+# @password-secret:         ID of a QCryptoSecret object providing a password
+#                           for authentication (defaults to no password)
+#
+# @proxy-username:          Username for proxy authentication (defaults to none)
+#
+# @proxy-password-secret:   ID of a QCryptoSecret object providing a password
+#                           for proxy authentication (defaults to no password)
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsCurlBase',
+  'data': { 'url': 'str',
+            '*readahead': 'int',
+            '*timeout': 'int',
+            '*username': 'str',
+            '*password-secret': 'str',
+            '*proxy-username': 'str',
+            '*proxy-password-secret': 'str' } }
+
+##
+# @BlockdevOptionsCurlHttp:
+#
+# Driver specific block device options for HTTP connections over the curl
+# backend.  URLs must start with "http://".
+#
+# @cookie:      List of cookies to set; format is
+#               "name1=content1; name2=content2;" as explained by
+#               CURLOPT_COOKIE(3). Defaults to no cookies.
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsCurlHttp',
+  'base': 'BlockdevOptionsCurlBase',
+  'data': { '*cookie': 'str' } }
+
+##
+# @BlockdevOptionsCurlHttps:
+#
+# Driver specific block device options for HTTPS connections over the curl
+# backend.  URLs must start with "https://".
+#
+# @cookie:      List of cookies to set; format is
+#               "name1=content1; name2=content2;" as explained by
+#               CURLOPT_COOKIE(3). Defaults to no cookies.
+#
+# @sslverify:   Whether to verify the SSL certificate's validity (defaults to
+#               true)
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsCurlHttps',
+  'base': 'BlockdevOptionsCurlBase',
+  'data': { '*cookie': 'str',
+            '*sslverify': 'bool' } }
+
+##
+# @BlockdevOptionsCurlFtp:
+#
+# Driver specific block device options for FTP connections over the curl
+# backend.  URLs must start with "ftp://".
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsCurlFtp',
+  'base': 'BlockdevOptionsCurlBase',
+  'data': { } }
+
+##
+# @BlockdevOptionsCurlFtps:
+#
+# Driver specific block device options for FTPS connections over the curl
+# backend.  URLs must start with "ftps://".
+#
+# @sslverify:   Whether to verify the SSL certificate's validity (defaults to
+#               true)
 #
 # Since: 2.9
 ##
-{ 'struct': 'BlockdevOptionsCurl',
-  'data': { 'filename': 'str' } }
+{ 'struct': 'BlockdevOptionsCurlFtps',
+  'base': 'BlockdevOptionsCurlBase',
+  'data': { '*sslverify': 'bool' } }
 
 ##
 # @BlockdevOptionsNbd:
@@ -2815,13 +2900,13 @@
       'cloop':      'BlockdevOptionsGenericFormat',
       'dmg':        'BlockdevOptionsGenericFormat',
       'file':       'BlockdevOptionsFile',
-      'ftp':        'BlockdevOptionsCurl',
-      'ftps':       'BlockdevOptionsCurl',
+      'ftp':        'BlockdevOptionsCurlFtp',
+      'ftps':       'BlockdevOptionsCurlFtps',
       'gluster':    'BlockdevOptionsGluster',
       'host_cdrom': 'BlockdevOptionsFile',
       'host_device':'BlockdevOptionsFile',
-      'http':       'BlockdevOptionsCurl',
-      'https':      'BlockdevOptionsCurl',
+      'http':       'BlockdevOptionsCurlHttp',
+      'https':      'BlockdevOptionsCurlHttps',
       'iscsi':      'BlockdevOptionsIscsi',
       'luks':       'BlockdevOptionsLUKS',
       'nbd':        'BlockdevOptionsNbd',
-- 
2.12.1

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9 1/2] qapi/curl: Extend and fix blockdev-add schema

[Jeff Cody]

On Fri, Mar 31, 2017 at 02:04:30PM +0200, Max Reitz wrote:
> The curl block driver accepts more options than just "filename"; also,
> the URL is actually expected to be passed through the "url" option
> instead of "filename".
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  qapi/block-core.json | 103 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 94 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b5f0e9958c..033457ce86 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2737,16 +2737,101 @@
>              '*debug': 'int' } }
>  
>  ##
> -# @BlockdevOptionsCurl:
> +# @BlockdevOptionsCurlBase:
>  #
> -# Driver specific block device options for the curl backend.
> +# Driver specific block device options shared by all protocols supported by the
> +# curl backend.
>  #
> -# @filename:    path to the image file
> +# @url:                     URL of the image file
> +#
> +# @readahead:               Size of the read-ahead cache; must be a multiple of
> +#                           512 (defaults to 256 kB)
> +#
> +# @timeout:                 Timeout for connections, in seconds (defaults to 5)
> +#
> +# @username:                Username for authentication (defaults to none)
> +#
> +# @password-secret:         ID of a QCryptoSecret object providing a password
> +#                           for authentication (defaults to no password)
> +#
> +# @proxy-username:          Username for proxy authentication (defaults to none)
> +#
> +# @proxy-password-secret:   ID of a QCryptoSecret object providing a password
> +#                           for proxy authentication (defaults to no password)
> +#
> +# Since: 2.9
> +##
> +{ 'struct': 'BlockdevOptionsCurlBase',
> +  'data': { 'url': 'str',
> +            '*readahead': 'int',
> +            '*timeout': 'int',
> +            '*username': 'str',
> +            '*password-secret': 'str',
> +            '*proxy-username': 'str',
> +            '*proxy-password-secret': 'str' } }
> +
> +##
> +# @BlockdevOptionsCurlHttp:
> +#
> +# Driver specific block device options for HTTP connections over the curl
> +# backend.  URLs must start with "http://".
> +#
> +# @cookie:      List of cookies to set; format is
> +#               "name1=content1; name2=content2;" as explained by
> +#               CURLOPT_COOKIE(3). Defaults to no cookies.
> +#
> +# Since: 2.9
> +##
> +{ 'struct': 'BlockdevOptionsCurlHttp',
> +  'base': 'BlockdevOptionsCurlBase',
> +  'data': { '*cookie': 'str' } }
> +
> +##
> +# @BlockdevOptionsCurlHttps:
> +#
> +# Driver specific block device options for HTTPS connections over the curl
> +# backend.  URLs must start with "https://".
> +#
> +# @cookie:      List of cookies to set; format is
> +#               "name1=content1; name2=content2;" as explained by
> +#               CURLOPT_COOKIE(3). Defaults to no cookies.
> +#
> +# @sslverify:   Whether to verify the SSL certificate's validity (defaults to
> +#               true)
> +#
> +# Since: 2.9
> +##
> +{ 'struct': 'BlockdevOptionsCurlHttps',
> +  'base': 'BlockdevOptionsCurlBase',
> +  'data': { '*cookie': 'str',
> +            '*sslverify': 'bool' } }
> +
> +##
> +# @BlockdevOptionsCurlFtp:
> +#
> +# Driver specific block device options for FTP connections over the curl
> +# backend.  URLs must start with "ftp://".
> +#
> +# Since: 2.9
> +##
> +{ 'struct': 'BlockdevOptionsCurlFtp',
> +  'base': 'BlockdevOptionsCurlBase',
> +  'data': { } }
> +
> +##
> +# @BlockdevOptionsCurlFtps:
> +#
> +# Driver specific block device options for FTPS connections over the curl
> +# backend.  URLs must start with "ftps://".
> +#
> +# @sslverify:   Whether to verify the SSL certificate's validity (defaults to
> +#               true)
>  #
>  # Since: 2.9
>  ##
> -{ 'struct': 'BlockdevOptionsCurl',
> -  'data': { 'filename': 'str' } }
> +{ 'struct': 'BlockdevOptionsCurlFtps',
> +  'base': 'BlockdevOptionsCurlBase',
> +  'data': { '*sslverify': 'bool' } }
>  
>  ##
>  # @BlockdevOptionsNbd:
> @@ -2815,13 +2900,13 @@
>        'cloop':      'BlockdevOptionsGenericFormat',
>        'dmg':        'BlockdevOptionsGenericFormat',
>        'file':       'BlockdevOptionsFile',
> -      'ftp':        'BlockdevOptionsCurl',
> -      'ftps':       'BlockdevOptionsCurl',
> +      'ftp':        'BlockdevOptionsCurlFtp',
> +      'ftps':       'BlockdevOptionsCurlFtps',
>        'gluster':    'BlockdevOptionsGluster',
>        'host_cdrom': 'BlockdevOptionsFile',
>        'host_device':'BlockdevOptionsFile',
> -      'http':       'BlockdevOptionsCurl',
> -      'https':      'BlockdevOptionsCurl',
> +      'http':       'BlockdevOptionsCurlHttp',
> +      'https':      'BlockdevOptionsCurlHttps',
>        'iscsi':      'BlockdevOptionsIscsi',
>        'luks':       'BlockdevOptionsLUKS',
>        'nbd':        'BlockdevOptionsNbd',
> -- 
> 2.12.1
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9 1/2] qapi/curl: Extend and fix blockdev-add schema

[Jeff Cody]

On Fri, Mar 31, 2017 at 02:04:30PM +0200, Max Reitz wrote:
> The curl block driver accepts more options than just "filename"; also,
> the URL is actually expected to be passed through the "url" option
> instead of "filename".
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/block-core.json | 103 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 94 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b5f0e9958c..033457ce86 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2737,16 +2737,101 @@
>              '*debug': 'int' } }
>  
>  ##
> -# @BlockdevOptionsCurl:
> +# @BlockdevOptionsCurlBase:
>  #
> -# Driver specific block device options for the curl backend.
> +# Driver specific block device options shared by all protocols supported by the
> +# curl backend.
>  #
> -# @filename:    path to the image file
> +# @url:                     URL of the image file
> +#
> +# @readahead:               Size of the read-ahead cache; must be a multiple of
> +#                           512 (defaults to 256 kB)
> +#
> +# @timeout:                 Timeout for connections, in seconds (defaults to 5)
> +#
> +# @username:                Username for authentication (defaults to none)
> +#
> +# @password-secret:         ID of a QCryptoSecret object providing a password
> +#                           for authentication (defaults to no password)
> +#
> +# @proxy-username:          Username for proxy authentication (defaults to none)
> +#
> +# @proxy-password-secret:   ID of a QCryptoSecret object providing a password
> +#                           for proxy authentication (defaults to no password)
> +#
> +# Since: 2.9
> +##
> +{ 'struct': 'BlockdevOptionsCurlBase',
> +  'data': { 'url': 'str',
> +            '*readahead': 'int',
> +            '*timeout': 'int',
> +            '*username': 'str',
> +            '*password-secret': 'str',
> +            '*proxy-username': 'str',
> +            '*proxy-password-secret': 'str' } }
> +
> +##
> +# @BlockdevOptionsCurlHttp:
> +#
> +# Driver specific block device options for HTTP connections over the curl
> +# backend.  URLs must start with "http://".
> +#
> +# @cookie:      List of cookies to set; format is
> +#               "name1=content1; name2=content2;" as explained by
> +#               CURLOPT_COOKIE(3). Defaults to no cookies.
> +#
> +# Since: 2.9
> +##
> +{ 'struct': 'BlockdevOptionsCurlHttp',
> +  'base': 'BlockdevOptionsCurlBase',
> +  'data': { '*cookie': 'str' } }
> +
> +##
> +# @BlockdevOptionsCurlHttps:
> +#
> +# Driver specific block device options for HTTPS connections over the curl
> +# backend.  URLs must start with "https://".
> +#
> +# @cookie:      List of cookies to set; format is
> +#               "name1=content1; name2=content2;" as explained by
> +#               CURLOPT_COOKIE(3). Defaults to no cookies.
> +#
> +# @sslverify:   Whether to verify the SSL certificate's validity (defaults to
> +#               true)
> +#
> +# Since: 2.9
> +##
> +{ 'struct': 'BlockdevOptionsCurlHttps',
> +  'base': 'BlockdevOptionsCurlBase',
> +  'data': { '*cookie': 'str',
> +            '*sslverify': 'bool' } }
> +
> +##
> +# @BlockdevOptionsCurlFtp:
> +#
> +# Driver specific block device options for FTP connections over the curl
> +# backend.  URLs must start with "ftp://".
> +#
> +# Since: 2.9
> +##
> +{ 'struct': 'BlockdevOptionsCurlFtp',
> +  'base': 'BlockdevOptionsCurlBase',
> +  'data': { } }
> +
> +##
> +# @BlockdevOptionsCurlFtps:
> +#
> +# Driver specific block device options for FTPS connections over the curl
> +# backend.  URLs must start with "ftps://".
> +#
> +# @sslverify:   Whether to verify the SSL certificate's validity (defaults to
> +#               true)
>  #
>  # Since: 2.9
>  ##
> -{ 'struct': 'BlockdevOptionsCurl',
> -  'data': { 'filename': 'str' } }
> +{ 'struct': 'BlockdevOptionsCurlFtps',
> +  'base': 'BlockdevOptionsCurlBase',
> +  'data': { '*sslverify': 'bool' } }
>  
>  ##
>  # @BlockdevOptionsNbd:
> @@ -2815,13 +2900,13 @@
>        'cloop':      'BlockdevOptionsGenericFormat',
>        'dmg':        'BlockdevOptionsGenericFormat',
>        'file':       'BlockdevOptionsFile',
> -      'ftp':        'BlockdevOptionsCurl',
> -      'ftps':       'BlockdevOptionsCurl',
> +      'ftp':        'BlockdevOptionsCurlFtp',
> +      'ftps':       'BlockdevOptionsCurlFtps',
>        'gluster':    'BlockdevOptionsGluster',
>        'host_cdrom': 'BlockdevOptionsFile',
>        'host_device':'BlockdevOptionsFile',
> -      'http':       'BlockdevOptionsCurl',
> -      'https':      'BlockdevOptionsCurl',
> +      'http':       'BlockdevOptionsCurlHttp',
> +      'https':      'BlockdevOptionsCurlHttps',
>        'iscsi':      'BlockdevOptionsIscsi',
>        'luks':       'BlockdevOptionsLUKS',
>        'nbd':        'BlockdevOptionsNbd',
> -- 
> 2.12.1
> 
>

I already applied this to my branch, but it occured to me that it would be
good to get a r-b from someone in the libvirt team (like Eric), with respect
to the schema.  So I'll hold off on sending a pull req for this series
for a bit.

-Jeff

Re: [Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: Extend and fix blockdev-add schema

[Eric Blake]

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

On 03/31/2017 07:04 AM, Max Reitz wrote:
> The curl block driver accepts more options than just "filename"; also,
> the URL is actually expected to be passed through the "url" option
> instead of "filename".
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/block-core.json | 103 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 94 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b5f0e9958c..033457ce86 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2737,16 +2737,101 @@
>              '*debug': 'int' } }
>  
>  ##
> -# @BlockdevOptionsCurl:
> +# @BlockdevOptionsCurlBase:
>  #
> -# Driver specific block device options for the curl backend.
> +# Driver specific block device options shared by all protocols supported by the
> +# curl backend.
>  #
> -# @filename:    path to the image file
> +# @url:                     URL of the image file
> +#
> +# @readahead:               Size of the read-ahead cache; must be a multiple of
> +#                           512 (defaults to 256 kB)
> +#
> +# @timeout:                 Timeout for connections, in seconds (defaults to 5)
> +#
> +# @username:                Username for authentication (defaults to none)
> +#
> +# @password-secret:         ID of a QCryptoSecret object providing a password
> +#                           for authentication (defaults to no password)
> +#
> +# @proxy-username:          Username for proxy authentication (defaults to none)
> +#
> +# @proxy-password-secret:   ID of a QCryptoSecret object providing a password
> +#                           for proxy authentication (defaults to no password)
> +#

Matches runtime_opts of block/curl.c, modulo the fields that you
restricted to specific protocols by creating subtypes. Looks like you
covered everything correctly.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix

[Max Reitz]

If the user has explicitly specified a block driver and thus a protocol,
we have to make sure the URL's protocol prefix matches. Otherwise the
latter will silently override the former which might catch some users by
surprise.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index 34dbd335f4..2708d57c2f 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -659,6 +659,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     const char *cookie;
     double d;
     const char *secretid;
+    const char *protocol_delimiter;
 
     static int inited = 0;
 
@@ -700,6 +701,15 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
         goto out_noclean;
     }
 
+    if (!strstart(file, bs->drv->protocol_name, &protocol_delimiter) ||
+        !strstart(protocol_delimiter, "://", NULL))
+    {
+        error_setg(errp, "%s curl driver cannot handle the URL '%s' (does not "
+                   "start with '%s://')", bs->drv->protocol_name, file,
+                   bs->drv->protocol_name);
+        goto out_noclean;
+    }
+
     s->username = g_strdup(qemu_opt_get(opts, CURL_BLOCK_OPT_USERNAME));
     secretid = qemu_opt_get(opts, CURL_BLOCK_OPT_PASSWORD_SECRET);
 
-- 
2.12.1

Re: [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix

[Philippe Mathieu-Daudé]

On 03/31/2017 09:04 AM, Max Reitz wrote:
> If the user has explicitly specified a block driver and thus a protocol,
> we have to make sure the URL's protocol prefix matches. Otherwise the
> latter will silently override the former which might catch some users by
> surprise.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  block/curl.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/block/curl.c b/block/curl.c
> index 34dbd335f4..2708d57c2f 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -659,6 +659,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>      const char *cookie;
>      double d;
>      const char *secretid;
> +    const char *protocol_delimiter;
>
>      static int inited = 0;
>
> @@ -700,6 +701,15 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>          goto out_noclean;
>      }
>
> +    if (!strstart(file, bs->drv->protocol_name, &protocol_delimiter) ||
> +        !strstart(protocol_delimiter, "://", NULL))
> +    {
> +        error_setg(errp, "%s curl driver cannot handle the URL '%s' (does not "
> +                   "start with '%s://')", bs->drv->protocol_name, file,
> +                   bs->drv->protocol_name);
> +        goto out_noclean;
> +    }
> +
>      s->username = g_strdup(qemu_opt_get(opts, CURL_BLOCK_OPT_USERNAME));
>      secretid = qemu_opt_get(opts, CURL_BLOCK_OPT_PASSWORD_SECRET);
>
>

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9 2/2] block/curl: Check protocol prefix

[Jeff Cody]

On Fri, Mar 31, 2017 at 02:04:31PM +0200, Max Reitz wrote:
> If the user has explicitly specified a block driver and thus a protocol,
> we have to make sure the URL's protocol prefix matches. Otherwise the
> latter will silently override the former which might catch some users by
> surprise.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/curl.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 34dbd335f4..2708d57c2f 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -659,6 +659,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>      const char *cookie;
>      double d;
>      const char *secretid;
> +    const char *protocol_delimiter;
>  
>      static int inited = 0;
>  
> @@ -700,6 +701,15 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>          goto out_noclean;
>      }
>  
> +    if (!strstart(file, bs->drv->protocol_name, &protocol_delimiter) ||
> +        !strstart(protocol_delimiter, "://", NULL))
> +    {
> +        error_setg(errp, "%s curl driver cannot handle the URL '%s' (does not "
> +                   "start with '%s://')", bs->drv->protocol_name, file,
> +                   bs->drv->protocol_name);
> +        goto out_noclean;
> +    }
> +
>      s->username = g_strdup(qemu_opt_get(opts, CURL_BLOCK_OPT_USERNAME));
>      secretid = qemu_opt_get(opts, CURL_BLOCK_OPT_PASSWORD_SECRET);
>  
> -- 
> 2.12.1
> 
> 

Re: [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix

[Eric Blake]

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

On 03/31/2017 07:04 AM, Max Reitz wrote:
> If the user has explicitly specified a block driver and thus a protocol,
> we have to make sure the URL's protocol prefix matches. Otherwise the
> latter will silently override the former which might catch some users by
> surprise.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 34dbd335f4..2708d57c2f 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -659,6 +659,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>      const char *cookie;
>      double d;
>      const char *secretid;
> +    const char *protocol_delimiter;
>  
>      static int inited = 0;
>  
> @@ -700,6 +701,15 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>          goto out_noclean;
>      }
>  
> +    if (!strstart(file, bs->drv->protocol_name, &protocol_delimiter) ||
> +        !strstart(protocol_delimiter, "://", NULL))

Do we care about case-insensitive comparisons here? But I'm fine with
case-sensitive for now, until we have an actual report of someone that
it breaks.

-- 
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 for-2.9 2/2] block/curl: Check protocol prefix

[Eric Blake]

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

On 03/31/2017 07:04 AM, Max Reitz wrote:
> If the user has explicitly specified a block driver and thus a protocol,
> we have to make sure the URL's protocol prefix matches. Otherwise the
> latter will silently override the former which might catch some users by
> surprise.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

It feels a little bit dirty that we are parsing the URL rather than
having a distinct discriminator, but I'm okay with the approach (with a
distinct discriminator, we would have to reconstruct a URL from the
discriminator + the rest of the server/path information, and that's more
work to libvirt to split a URL into the distinct JSON fields just to
have qemu rebuild a URL).

> +    if (!strstart(file, bs->drv->protocol_name, &protocol_delimiter) ||
> +        !strstart(protocol_delimiter, "://", NULL))
> +    {
> +        error_setg(errp, "%s curl driver cannot handle the URL '%s' (does not "
> +                   "start with '%s://')", bs->drv->protocol_name, file,
> +                   bs->drv->protocol_name);

Perhaps splitting the message with an error_append_hint() for the
parenthetical half would also be appropriate, but the line is not too
terribly long so I won't insist on a respin.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 for-2.9 2/2] block/curl: Check protocol prefix

[Max Reitz]

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

On 31.03.2017 21:30, Eric Blake wrote:
> On 03/31/2017 07:04 AM, Max Reitz wrote:
>> If the user has explicitly specified a block driver and thus a protocol,
>> we have to make sure the URL's protocol prefix matches. Otherwise the
>> latter will silently override the former which might catch some users by
>> surprise.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/curl.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
> 
> It feels a little bit dirty that we are parsing the URL rather than
> having a distinct discriminator, but I'm okay with the approach (with a
> distinct discriminator, we would have to reconstruct a URL from the
> discriminator + the rest of the server/path information, and that's more
> work to libvirt to split a URL into the distinct JSON fields just to
> have qemu rebuild a URL).

Yes, you're right, there are a couple of points where the interface
could be nicer. Another thing is the cookie string which could be a list
of dicts or something similar. But in the end all of this block driver
really is just an interface for libcurl, so I thought it best to just
behave as such (i.e. take "blob" strings that are then just piped into
libcurl).

(The distinct discriminator would probably be just the block driver
name. But then every user would have to strip the protocol part from the
URL...)

>> +    if (!strstart(file, bs->drv->protocol_name, &protocol_delimiter) ||
>> +        !strstart(protocol_delimiter, "://", NULL))

Regarding case sensitivity: I have to say that I actually can't remember
when I saw a not full lower-cased protocol specified in a URL, and I'm
glad about that. :-)

I also think we can wait until someone files a bug (at which point we
can always ask them how hard it would be to just write the protocol in
lower case...).

>> +    {
>> +        error_setg(errp, "%s curl driver cannot handle the URL '%s' (does not "
>> +                   "start with '%s://')", bs->drv->protocol_name, file,
>> +                   bs->drv->protocol_name);
> 
> Perhaps splitting the message with an error_append_hint() for the
> parenthetical half would also be appropriate, but the line is not too
> terribly long so I won't insist on a respin.

Good idea. But I don't think it's quite worth a respin either at this point.

(Also, it can be argued that the part in parentheses is in fact the
actual error source, so it kind of makes sense to keep it in the main
error message.)

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Max


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

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema

[Jeff Cody]

On Fri, Mar 31, 2017 at 02:04:29PM +0200, Max Reitz wrote:
> Yes, it's yet another episode in our popular
> get-blockdev-add-ready-for-2.9 drama!
> 
> Right now, the schema for the curl block driver is seriously lacking.
> This series improves things at least a bit.
> 
> To improve things seriously, we might want to structure the URL instead
> of it being just a plain string, and we might want to split the cookie
> string into a list of dicts or something similar. However, strictly
> speaking our curl block driver is *not* an (ht|f)tps? block driver but
> just a curl driver. All it does is pass some options to libcurl and then
> send and receive data from it. (We really should have just named it
> "curl" from the start.)
> 
> Therefore, it probably is for the best to leave these options rather
> opaque and let libcurl do the interpretation.
> 
> 
> Max Reitz (2):
>   qapi/curl: Extend and fix blockdev-add schema
>   block/curl: Check protocol prefix
> 
>  qapi/block-core.json | 103 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  block/curl.c         |  10 +++++
>  2 files changed, 104 insertions(+), 9 deletions(-)
> 
> -- 
> 2.12.1
> 
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff