Message ID | 20171120201004.14999-25-mreitz@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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
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
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
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
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
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
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 --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, };
Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/curl.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)