diff mbox series

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

Message ID 20171120201004.14999-25-mreitz@redhat.com
State New
Headers show
Series block: Fix some filename generation issues | expand

Commit Message

Max Reitz Nov. 20, 2017, 8:10 p.m. UTC 
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Alberto Garcia Dec. 4, 2017, 4:51 p.m. UTC | #1 
On Mon 20 Nov 2017 09:10:03 PM CET, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  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
Max Reitz Dec. 4, 2017, 6:26 p.m. UTC | #2 
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 <mreitz@redhat.com>
>> ---
>>  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
Alberto Garcia Dec. 5, 2017, 10:31 a.m. UTC | #3 
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
Alberto Garcia Dec. 5, 2017, 11:45 a.m. UTC | #4 
On Mon 20 Nov 2017 09:10:03 PM CET, Max Reitz <mreitz@redhat.com> wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  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@igalia.com>

Berto
Max Reitz Dec. 8, 2017, 1:47 p.m. UTC | #5 
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
Daniel P. Berrangé Dec. 8, 2017, 1:53 p.m. UTC | #6 
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

Regards,
Daniel
Max Reitz Dec. 8, 2017, 2:12 p.m. UTC | #7 
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
Alberto Garcia Dec. 8, 2017, 4:14 p.m. UTC | #8 
On Fri 08 Dec 2017 02:47:52 PM CET, Max Reitz <mreitz@redhat.com> 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
diff mbox series

Patch

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,
 };