[Qemu-devel] [PATCH] curl: Don't set curl options on the handle just before it's going to be deleted.

[Qemu-devel] [PATCH] curl: Don't set curl options on the handle just before it's going to be deleted.

[Richard W.M. Jones]

From: "Richard W.M. Jones" <rjones@redhat.com>

(Found by Kamil Dudka)

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 block/curl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index b634ccf..bf31efe 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -453,7 +453,6 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
         goto out;
     curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
     curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_read_cb);
-    curl_easy_setopt(state->curl, CURLOPT_NOBODY, 0);
     if (d)
         s->len = (size_t)d;
     else if(!s->len)
-- 
1.8.2.1

Re: [Qemu-devel] [Qemu-trivial] [PATCH] curl: Don't set curl options on the handle just before it's going to be deleted.

[Michael Tokarev]

10.06.2013 17:19, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones" <rjones@redhat.com>
> 
> (Found by Kamil Dudka)
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  block/curl.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index b634ccf..bf31efe 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -453,7 +453,6 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
>          goto out;
>      curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
>      curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_read_cb);
> -    curl_easy_setopt(state->curl, CURLOPT_NOBODY, 0);
>      if (d)
>          s->len = (size_t)d;
>      else if(!s->len)

Adding a bit more context:


    curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
    curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_size_cb);
    if (curl_easy_perform(state->curl))
        goto out;
    curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
    curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_read_cb);
=>> curl_easy_setopt(state->curl, CURLOPT_NOBODY, 0);
    if (d)
        s->len = (size_t)d;
    else if(!s->len)
        goto out;
    DPRINTF("CURL: Size = %zd\n", s->len);

    curl_clean_state(state);
    curl_easy_cleanup(state->curl);
    state->curl = NULL;

So indeed, the curl object is being removed here a few lines down the code.

But it looks like the previous call to curl_easy_setopt, to set WRITEFUNCTION,
is also not very useful. Should we probably want to remove that too, or don't
remove either of these?

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH] curl: Don't set curl options on the handle just before it's going to be deleted.

[Richard W.M. Jones]

On Mon, Jun 10, 2013 at 07:34:52PM +0400, Michael Tokarev wrote:
> 10.06.2013 17:19, Richard W.M. Jones wrote:
> > From: "Richard W.M. Jones" <rjones@redhat.com>
> > 
> > (Found by Kamil Dudka)
> > 
> > Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > ---
> >  block/curl.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/block/curl.c b/block/curl.c
> > index b634ccf..bf31efe 100644
> > --- a/block/curl.c
> > +++ b/block/curl.c
> > @@ -453,7 +453,6 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
> >          goto out;
> >      curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
> >      curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_read_cb);
> > -    curl_easy_setopt(state->curl, CURLOPT_NOBODY, 0);
> >      if (d)
> >          s->len = (size_t)d;
> >      else if(!s->len)
> 
> Adding a bit more context:
> 
> 
>     curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
>     curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_size_cb);
>     if (curl_easy_perform(state->curl))
>         goto out;
>     curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
>     curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_read_cb);
> =>> curl_easy_setopt(state->curl, CURLOPT_NOBODY, 0);
>     if (d)
>         s->len = (size_t)d;
>     else if(!s->len)
>         goto out;
>     DPRINTF("CURL: Size = %zd\n", s->len);
> 
>     curl_clean_state(state);
>     curl_easy_cleanup(state->curl);
>     state->curl = NULL;
> 
> So indeed, the curl object is being removed here a few lines down the code.
> 
> But it looks like the previous call to curl_easy_setopt, to set WRITEFUNCTION,
> is also not very useful. Should we probably want to remove that too, or don't
> remove either of these?

Yes you're right.  For some reason I "saw" another call to
curl_easy_perform which obviously isn't there in reality.  Will send a
v2 patch shortly after I've done a bit of testing.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
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