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 <address@hidden>
> > Reviewed-by: Laszlo Ersek <address@hidden>
> > ---
> >  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