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