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

[Alberto Garcia]

On Fri 08 Dec 2017 02:47:52 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.

Indeed, I though it would perhaps be possible to create a curl BDS to
this without having to perform an actual connection, but never mind if
it's not possible / too complicated.

> Also, adding macros for the default values could help, I think.

Yep.

Berto

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

[Max Reitz]

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.

And just adding a test for all protocols that then accesses qemu.org
feels somehow wrong, too...  Maybe if I add it to a network group, that
wouldn't feel so bad.

Also, adding macros for the default values could help, I think.

Max



signature.asc
Description: OpenPGP digital signature

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

[Alberto Garcia]

On Mon 20 Nov 2017 09:10:03 PM CET, Max Reitz  wrote:
> Signed-off-by: Max Reitz 
> ---
>  block/curl.c | 18 ++
>  1 file changed, 18 insertions(+)
>
> diff --git a/block/curl.c b/block/curl.c
> index 11318a9a29..fe57223fda 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -957,6 +957,20 @@ static int64_t curl_getlength(BlockDriverState *bs)
>  return s->len;
>  }
>  
> +static void curl_refresh_filename(BlockDriverState *bs)
> +{
> +BDRVCURLState *s = bs->opaque;
> +
> +if (!s->sslverify || s->cookie ||
> +s->username || s->password || s->proxyusername || s->proxypassword)
> +{
> +return;
> +}
> +
> +pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), s->url);
> +}

Reviewed-by: Alberto Garcia 

Berto

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

[Alberto Garcia]

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.

I understand that the default value is never supposed to change so in
practice I don't see this breaking, but is it perhaps worth adding tests
for all these cases?

Berto

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

[Max Reitz]

On 2017-12-04 17:51, Alberto Garcia wrote:
> On Mon 20 Nov 2017 09:10:03 PM CET, Max Reitz wrote:
>> Signed-off-by: Max Reitz 
>> ---
>>  block/curl.c | 18 ++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 11318a9a29..fe57223fda 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -957,6 +957,20 @@ static int64_t curl_getlength(BlockDriverState *bs)
>>  return s->len;
>>  }
>>  
>> +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).

Thanks for reviewing, again. :-)

Max



signature.asc
Description: OpenPGP digital signature

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

[Alberto Garcia]

On Mon 20 Nov 2017 09:10:03 PM CET, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  block/curl.c | 18 ++
>  1 file changed, 18 insertions(+)
>
> diff --git a/block/curl.c b/block/curl.c
> index 11318a9a29..fe57223fda 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -957,6 +957,20 @@ static int64_t curl_getlength(BlockDriverState *bs)
>  return s->len;
>  }
>  
> +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?

Berto

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

[Max Reitz]

Signed-off-by: Max Reitz 
---
 block/curl.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index 11318a9a29..fe57223fda 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -957,6 +957,20 @@ static int64_t curl_getlength(BlockDriverState *bs)
 return s->len;
 }
 
+static void curl_refresh_filename(BlockDriverState *bs)
+{
+BDRVCURLState *s = bs->opaque;
+
+if (!s->sslverify || s->cookie ||
+s->username || s->password || s->proxyusername || s->proxypassword)
+{
+return;
+}
+
+pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), s->url);
+}
+
+
 static const char *const curl_sgfnt_runtime_opts[] = {
 CURL_BLOCK_OPT_URL,
 CURL_BLOCK_OPT_SSLVERIFY,
@@ -985,6 +999,7 @@ static BlockDriver bdrv_http = {
 .bdrv_detach_aio_context= curl_detach_aio_context,
 .bdrv_attach_aio_context= curl_attach_aio_context,
 
+.bdrv_refresh_filename  = curl_refresh_filename,
 .sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
 };
 
@@ -1003,6 +1018,7 @@ static BlockDriver bdrv_https = {
 .bdrv_detach_aio_context= curl_detach_aio_context,
 .bdrv_attach_aio_context= curl_attach_aio_context,
 
+.bdrv_refresh_filename  = curl_refresh_filename,
 .sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
 };
 
@@ -1021,6 +1037,7 @@ static BlockDriver bdrv_ftp = {
 .bdrv_detach_aio_context= curl_detach_aio_context,
 .bdrv_attach_aio_context= curl_attach_aio_context,
 
+.bdrv_refresh_filename  = curl_refresh_filename,
 .sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
 };
 
@@ -1039,6 +1056,7 @@ static BlockDriver bdrv_ftps = {
 .bdrv_detach_aio_context= curl_detach_aio_context,
 .bdrv_attach_aio_context= curl_attach_aio_context,
 
+.bdrv_refresh_filename  = curl_refresh_filename,
 .sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
 };
 
-- 
2.13.6