Re: [Qemu-devel] [PATCH v7 for-2.12 24/25] block/curl: Implement bdrv_refresh_filename()

[Max Reitz]

On 2017-12-08 14:53, Daniel P. Berrange wrote:
> On Fri, Dec 08, 2017 at 02:47:52PM +0100, Max Reitz wrote:
>> On 2017-12-05 11:31, Alberto Garcia wrote:
>>> On Mon 04 Dec 2017 07:26:02 PM CET, Max Reitz wrote:
>>>>>> +static void curl_refresh_filename(BlockDriverState *bs)
>>>>>> +{
>>>>>> +    BDRVCURLState *s = bs->opaque;
>>>>>> +
>>>>>> +    if (!s->sslverify || s->cookie ||
>>>>>> +        s->username || s->password || s->proxyusername || 
>>>>>> s->proxypassword)
>>>>>> +    {
>>>>>
>>>>> Is !s->sslverify negative because that setting is true by default?
>>>>
>>>> Yes, exactly.  If it's false, you'd need to override it (and you can't
>>>> do that through a plain filename).
>>>
>>> I think this is not the only case in this series, but I'm not very
>>> comfortable with the idea that this condition and the default value of
>>> the setting are implicity dependent on each other. If you change one and
>>> forget to change the other things will break.
>>
>> Well, yes, but...
>>
>>> I understand that the default value is never supposed to change so in
>>> practice I don't see this breaking,
>>
>> Yes.
>>
>>>                                     but is it perhaps worth adding tests
>>> for all these cases?
>>
>> In theory, sure.  In practice, adding a curl test case seems hard.
>>
>> Well, I could connect to, I don't know, qemu.org or something and then
>> just test things there (with the index used as a raw image), but if I
>> add one test for the curl protocol, nobody is ever going to run them...
>> And in theory, that's not how a curl test should work.  In theory, you'd
>> expect the user to set up a localhost server with some known root
>> directory and then _make_test_img etc. would set up images there.  But
>> that way, really nobody is ever going to run them.
> 
> For qemu I/O tests I would suggest just having the iotest spin up a simple
> http server in Python - there's standard APIs in python that make this
> pretty trivial
> 
>   
> http://www.pythonforbeginners.com/modules-in-python/how-to-use-simplehttpserver/
> 
> That way curl could be reliably tested without any special setup by the
> developer

Good idea, thanks.  It might be an issue if we ever want to test HTTPS
(putting a self-signed certificate into the iotests directory seems a
bit over the top), but since the sslverify option works just fine with
HTTP itself, that's not an issue for this case.

Now I just have to discuss with myself whether I want to add curl
testing infrastructure for testing whether the defaults are still the
defaults or whether adding default value macros is enough...

Max

signature.asc
Description: OpenPGP digital signature