[Qemu-devel] [PATCH v1 repost 2] block/curl: Improve type safety of s->timeout.

[Qemu-devel] [PATCH v1 repost 2] block/curl: Improve type safety of s->timeout.

[Richard W.M. Jones]

No change since previous repost, except I have rebased it against git
head.

Three line change for obvious bug.  Can I get another review?

Previous discussion:
https://lists.gnu.org/archive/html/qemu-devel/2014-10/threads.html#00518

Rich.

[Qemu-devel] [PATCH v1 repost 2] block/curl: Improve type safety of s->timeout.

[Richard W.M. Jones]

qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a
long (not an int).

Store the timeout (which is a positive number of seconds) as a
uint64_t.  Check that the number given by the user is reasonable.
Cast it to long before calling curl_easy_setopt.

Example error message after this change has been applied:

$ ./qemu-img create -f qcow2 /tmp/test.qcow2 \
    -b 'json: { "file.driver":"https",
                "file.url":"https://foo/bar",
                "file.timeout":-1 }'
qemu-img: /tmp/test.qcow2: Could not open 'json: { "file.driver":"https", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout parameter is too large or negative: Invalid argument

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 block/curl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index b4157cc..2b40802 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -112,7 +112,7 @@ typedef struct BDRVCURLState {
     char *url;
     size_t readahead_size;
     bool sslverify;
-    int timeout;
+    uint64_t timeout;
     char *cookie;
     bool accept_range;
     AioContext *aio_context;
@@ -390,7 +390,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
         if (s->cookie) {
             curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
         }
-        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, s->timeout);
+        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout);
         curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
                          (void *)curl_read_cb);
         curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
@@ -546,6 +546,10 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->timeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
                                      CURL_TIMEOUT_DEFAULT);
+    if (s->timeout > 100000) {
+        error_setg(errp, "timeout parameter is too large or negative");
+        goto out_noclean;
+    }
 
     s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
 
-- 
2.0.4

Re: [Qemu-devel] [PATCH v1 repost 2] block/curl: Improve type safety of s->timeout.

[Gonglei]

On 2014/10/26 16:42, Richard W.M. Jones wrote:

> qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a
> long (not an int).
> 
> Store the timeout (which is a positive number of seconds) as a
> uint64_t.  Check that the number given by the user is reasonable.
> Cast it to long before calling curl_easy_setopt.
> 
> Example error message after this change has been applied:
> 
> $ ./qemu-img create -f qcow2 /tmp/test.qcow2 \
>     -b 'json: { "file.driver":"https",
>                 "file.url":"https://foo/bar",
>                 "file.timeout":-1 }'
> qemu-img: /tmp/test.qcow2: Could not open 'json: { "file.driver":"https", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout parameter is too large or negative: Invalid argument
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  block/curl.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index b4157cc..2b40802 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -112,7 +112,7 @@ typedef struct BDRVCURLState {
>      char *url;
>      size_t readahead_size;
>      bool sslverify;
> -    int timeout;
> +    uint64_t timeout;
>      char *cookie;
>      bool accept_range;
>      AioContext *aio_context;
> @@ -390,7 +390,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
>          if (s->cookie) {
>              curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
>          }
> -        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, s->timeout);
> +        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout);
>          curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
>                           (void *)curl_read_cb);
>          curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
> @@ -546,6 +546,10 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      s->timeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
>                                       CURL_TIMEOUT_DEFAULT);
> +    if (s->timeout > 100000) {

why 100000? And it's a magic number.

Best regards,
-Gonglei

> +        error_setg(errp, "timeout parameter is too large or negative");
> +        goto out_noclean;
> +    }
>  
>      s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
>  

Re: [Qemu-devel] [PATCH v1 repost 2] block/curl: Improve type safety of s->timeout.

[Richard W.M. Jones]

On Sun, Oct 26, 2014 at 04:57:46PM +0800, Gonglei wrote:
> On 2014/10/26 16:42, Richard W.M. Jones wrote:
> 
> > qemu_opt_get_number returns a uint64_t, and curl_easy_setopt expects a
> > long (not an int).
> > 
> > Store the timeout (which is a positive number of seconds) as a
> > uint64_t.  Check that the number given by the user is reasonable.
> > Cast it to long before calling curl_easy_setopt.
> > 
> > Example error message after this change has been applied:
> > 
> > $ ./qemu-img create -f qcow2 /tmp/test.qcow2 \
> >     -b 'json: { "file.driver":"https",
> >                 "file.url":"https://foo/bar",
> >                 "file.timeout":-1 }'
> > qemu-img: /tmp/test.qcow2: Could not open 'json: { "file.driver":"https", "file.url":"https://foo/bar", "file.timeout":-1 }': timeout parameter is too large or negative: Invalid argument
> > 
> > Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  block/curl.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/curl.c b/block/curl.c
> > index b4157cc..2b40802 100644
> > --- a/block/curl.c
> > +++ b/block/curl.c
> > @@ -112,7 +112,7 @@ typedef struct BDRVCURLState {
> >      char *url;
> >      size_t readahead_size;
> >      bool sslverify;
> > -    int timeout;
> > +    uint64_t timeout;
> >      char *cookie;
> >      bool accept_range;
> >      AioContext *aio_context;
> > @@ -390,7 +390,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
> >          if (s->cookie) {
> >              curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
> >          }
> > -        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, s->timeout);
> > +        curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout);
> >          curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION,
> >                           (void *)curl_read_cb);
> >          curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
> > @@ -546,6 +546,10 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
> >  
> >      s->timeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
> >                                       CURL_TIMEOUT_DEFAULT);
> > +    if (s->timeout > 100000) {
> 
> why 100000? And it's a magic number.

It's just there to stop unreasonable timeouts or negative numbers.
100000 s is 27 hours, and no webserver I know of would keep a
connection open that long.  Possibly not even the IP stack.

What's the difference between defining a number at the top of the file
to be used once, and placing it exactly where it is used?  Except the
former introduces long range dependencies into the code making it
harder to read and more fragile when changed.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Re: [Qemu-devel] [PATCH v1 repost 2] block/curl: Improve type safety of s->timeout.

[Gonglei]

On 2014/10/26 18:22, Richard W.M. Jones wrote:

> It's just there to stop unreasonable timeouts or negative numbers.
> 100000 s is 27 hours, and no webserver I know of would keep a
> connection open that long.  Possibly not even the IP stack.
> 

Yes, it is. But 26 hours is OK? I just think we should assure the timeout
as reasonable range, absolutely 100000 is too big IMO.

> What's the difference between defining a number at the top of the file
> to be used once, and placing it exactly where it is used?  Except the
> former introduces long range dependencies into the code making it
> harder to read and more fragile when changed.


That's the purpose using macro. If this value is used only one place  in the
curl.c (or other c files) now and future, you are fine with it. :)

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v1 repost 2] block/curl: Improve type safety of s->timeout.

[Richard W.M. Jones]

On Sun, Oct 26, 2014 at 06:45:02PM +0800, Gonglei wrote:
> On 2014/10/26 18:22, Richard W.M. Jones wrote:
> 
> > It's just there to stop unreasonable timeouts or negative numbers.
> > 100000 s is 27 hours, and no webserver I know of would keep a
> > connection open that long.  Possibly not even the IP stack.
> > 
> 
> Yes, it is. But 26 hours is OK? I just think we should assure the timeout
> as reasonable range, absolutely 100000 is too big IMO.
> 
> > What's the difference between defining a number at the top of the file
> > to be used once, and placing it exactly where it is used?  Except the
> > former introduces long range dependencies into the code making it
> > harder to read and more fragile when changed.
> 
> 
> That's the purpose using macro. If this value is used only one place  in the
> curl.c (or other c files) now and future, you are fine with it. :)

I don't understand this part.  Can you explain how you think a macro
should be used?

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [Qemu-devel] [PATCH v1 repost 2] block/curl: Improve type safety of s->timeout.

[Gonglei]

On 2014/10/26 18:48, Richard W.M. Jones wrote:

> On Sun, Oct 26, 2014 at 06:45:02PM +0800, Gonglei wrote:
>> On 2014/10/26 18:22, Richard W.M. Jones wrote:
>>
>>> It's just there to stop unreasonable timeouts or negative numbers.
>>> 100000 s is 27 hours, and no webserver I know of would keep a
>>> connection open that long.  Possibly not even the IP stack.
>>>
>>
>> Yes, it is. But 26 hours is OK? I just think we should assure the timeout
>> as reasonable range, absolutely 100000 is too big IMO.
>>
>>> What's the difference between defining a number at the top of the file
>>> to be used once, and placing it exactly where it is used?  Except the
>>> former introduces long range dependencies into the code making it
>>> harder to read and more fragile when changed.
>>
>>
>> That's the purpose using macro. If this value is used only one place  in the
>> curl.c (or other c files) now and future, you are fine with it. :)
> 
> I don't understand this part.  Can you explain how you think a macro
> should be used?
> 

Sorry for misapprehension. I mean that's  the purpose using macro what your said:
" Except the former introduces long range dependencies into the code making it
harder to read and more fragile when changed."

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v1 repost 2] block/curl: Improve type safety of s->timeout.

[Richard W.M. Jones]

On Sun, Oct 26, 2014 at 06:55:21PM +0800, Gonglei wrote:
> On 2014/10/26 18:48, Richard W.M. Jones wrote:
> 
> > On Sun, Oct 26, 2014 at 06:45:02PM +0800, Gonglei wrote:
> >> On 2014/10/26 18:22, Richard W.M. Jones wrote:
> >>
> >>> It's just there to stop unreasonable timeouts or negative numbers.
> >>> 100000 s is 27 hours, and no webserver I know of would keep a
> >>> connection open that long.  Possibly not even the IP stack.
> >>>
> >>
> >> Yes, it is. But 26 hours is OK? I just think we should assure the timeout
> >> as reasonable range, absolutely 100000 is too big IMO.
> >>
> >>> What's the difference between defining a number at the top of the file
> >>> to be used once, and placing it exactly where it is used?  Except the
> >>> former introduces long range dependencies into the code making it
> >>> harder to read and more fragile when changed.
> >>
> >>
> >> That's the purpose using macro. If this value is used only one place  in the
> >> curl.c (or other c files) now and future, you are fine with it. :)
> > 
> > I don't understand this part.  Can you explain how you think a macro
> > should be used?
> > 
> 
> Sorry for misapprehension. I mean that's  the purpose using macro what your said:
> " Except the former introduces long range dependencies into the code making it
> harder to read and more fragile when changed."

OK, so our coding style is about introducing fragility and long
range dependencies.  v2 coming up ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v