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

[Max Reitz]

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 <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks!

Max

signature.asc
Description: OpenPGP digital signature