| Message ID | 20170331120431.1767-3-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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); > >
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 > >
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.
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>
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
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);
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(+)