* [Qemu-devel] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema @ 2017-03-31 12:04 Max Reitz 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: " Max Reitz ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Max Reitz @ 2017-03-31 12:04 UTC (permalink / raw) To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: Extend and fix blockdev-add schema 2017-03-31 12:04 [Qemu-devel] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema Max Reitz @ 2017-03-31 12:04 ` Max Reitz 2017-03-31 18:57 ` [Qemu-devel] [Qemu-block] " Jeff Cody ` (2 more replies) 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix Max Reitz 2017-03-31 19:00 ` [Qemu-devel] [Qemu-block] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema Jeff Cody 2 siblings, 3 replies; 12+ messages in thread From: Max Reitz @ 2017-03-31 12:04 UTC (permalink / raw) To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9 1/2] qapi/curl: Extend and fix blockdev-add schema 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: " Max Reitz @ 2017-03-31 18:57 ` Jeff Cody 2017-03-31 19:27 ` Jeff Cody 2017-03-31 19:27 ` [Qemu-devel] " Eric Blake 2 siblings, 0 replies; 12+ messages in thread From: Jeff Cody @ 2017-03-31 18:57 UTC (permalink / raw) To: Max Reitz; +Cc: qemu-block, Kevin Wolf, qemu-devel 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9 1/2] qapi/curl: Extend and fix blockdev-add schema 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: " Max Reitz 2017-03-31 18:57 ` [Qemu-devel] [Qemu-block] " Jeff Cody @ 2017-03-31 19:27 ` Jeff Cody 2017-03-31 19:27 ` [Qemu-devel] " Eric Blake 2 siblings, 0 replies; 12+ messages in thread From: Jeff Cody @ 2017-03-31 19:27 UTC (permalink / raw) To: Max Reitz; +Cc: qemu-block, Kevin Wolf, qemu-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: Extend and fix blockdev-add schema 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: " Max Reitz 2017-03-31 18:57 ` [Qemu-devel] [Qemu-block] " Jeff Cody 2017-03-31 19:27 ` Jeff Cody @ 2017-03-31 19:27 ` Eric Blake 2 siblings, 0 replies; 12+ messages in thread From: Eric Blake @ 2017-03-31 19:27 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix 2017-03-31 12:04 [Qemu-devel] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema Max Reitz 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: " Max Reitz @ 2017-03-31 12:04 ` Max Reitz 2017-03-31 13:52 ` Philippe Mathieu-Daudé ` (3 more replies) 2017-03-31 19:00 ` [Qemu-devel] [Qemu-block] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema Jeff Cody 2 siblings, 4 replies; 12+ messages in thread From: Max Reitz @ 2017-03-31 12:04 UTC (permalink / raw) To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix Max Reitz @ 2017-03-31 13:52 ` Philippe Mathieu-Daudé 2017-03-31 18:58 ` [Qemu-devel] [Qemu-block] " Jeff Cody ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Philippe Mathieu-Daudé @ 2017-03-31 13:52 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel 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); > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9 2/2] block/curl: Check protocol prefix 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix Max Reitz 2017-03-31 13:52 ` Philippe Mathieu-Daudé @ 2017-03-31 18:58 ` Jeff Cody 2017-03-31 19:14 ` [Qemu-devel] " Eric Blake 2017-03-31 19:30 ` Eric Blake 3 siblings, 0 replies; 12+ messages in thread From: Jeff Cody @ 2017-03-31 18:58 UTC (permalink / raw) To: Max Reitz; +Cc: qemu-block, Kevin Wolf, qemu-devel 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix Max Reitz 2017-03-31 13:52 ` Philippe Mathieu-Daudé 2017-03-31 18:58 ` [Qemu-devel] [Qemu-block] " Jeff Cody @ 2017-03-31 19:14 ` Eric Blake 2017-03-31 19:30 ` Eric Blake 3 siblings, 0 replies; 12+ messages in thread From: Eric Blake @ 2017-03-31 19:14 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix Max Reitz ` (2 preceding siblings ...) 2017-03-31 19:14 ` [Qemu-devel] " Eric Blake @ 2017-03-31 19:30 ` Eric Blake 2017-03-31 20:50 ` Max Reitz 3 siblings, 1 reply; 12+ messages in thread From: Eric Blake @ 2017-03-31 19:30 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix 2017-03-31 19:30 ` Eric Blake @ 2017-03-31 20:50 ` Max Reitz 0 siblings, 0 replies; 12+ messages in thread From: Max Reitz @ 2017-03-31 20:50 UTC (permalink / raw) To: Eric Blake, qemu-block; +Cc: qemu-devel, Kevin Wolf [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema 2017-03-31 12:04 [Qemu-devel] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema Max Reitz 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: " Max Reitz 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix Max Reitz @ 2017-03-31 19:00 ` Jeff Cody 2 siblings, 0 replies; 12+ messages in thread From: Jeff Cody @ 2017-03-31 19:00 UTC (permalink / raw) To: Max Reitz; +Cc: qemu-block, Kevin Wolf, qemu-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-03-31 20:51 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-31 12:04 [Qemu-devel] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema Max Reitz 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 1/2] qapi/curl: " Max Reitz 2017-03-31 18:57 ` [Qemu-devel] [Qemu-block] " Jeff Cody 2017-03-31 19:27 ` Jeff Cody 2017-03-31 19:27 ` [Qemu-devel] " Eric Blake 2017-03-31 12:04 ` [Qemu-devel] [PATCH for-2.9 2/2] block/curl: Check protocol prefix Max Reitz 2017-03-31 13:52 ` Philippe Mathieu-Daudé 2017-03-31 18:58 ` [Qemu-devel] [Qemu-block] " Jeff Cody 2017-03-31 19:14 ` [Qemu-devel] " Eric Blake 2017-03-31 19:30 ` Eric Blake 2017-03-31 20:50 ` Max Reitz 2017-03-31 19:00 ` [Qemu-devel] [Qemu-block] [PATCH for-2.9 0/2] curl: Extend and fix blockdev-add schema Jeff Cody
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.