[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 for-2.12 24/25] block/curl: Implement bdrv_re
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v7 for-2.12 24/25] block/curl: Implement bdrv_refresh_filename() |
Date: |
Fri, 8 Dec 2017 15:12:42 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
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