Patchwork [PULL,28/31] curl: Remove unnecessary explicit calls to internal event handler

login
register
mail settings
Submitter Kevin Wolf
Date April 30, 2014, 6:24 p.m.
Message ID <1398882243-14783-29-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/344277/
State New
Headers show

Comments

Kevin Wolf - April 30, 2014, 6:24 p.m.
From: Matthew Booth <mbooth@redhat.com>

Remove calls to curl_multi_do where the relevant handles are already
registered to the event loop.

Ensure that we kick off socket handling with CURL_SOCKET_TIMEOUT after
adding a new handle.

Signed-off-by: Matthew Booth <mbooth@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/curl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Laurent Desnogues - May 6, 2014, 9:44 a.m.
Hello,

sorry for another late detection of the use of too recent features.

On Wed, Apr 30, 2014 at 8:24 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> From: Matthew Booth <mbooth@redhat.com>
>
> Remove calls to curl_multi_do where the relevant handles are already
> registered to the event loop.
>
> Ensure that we kick off socket handling with CURL_SOCKET_TIMEOUT after
> adding a new handle.
>
> Signed-off-by: Matthew Booth <mbooth@redhat.com>
> Tested-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/curl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 50bd05f..fd2756e 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -535,7 +535,6 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>      curl_multi_setopt(s->multi, CURLMOPT_TIMERDATA, s);
>      curl_multi_setopt(s->multi, CURLMOPT_TIMERFUNCTION, curl_timer_cb);
>  #endif
> -    curl_multi_do(s);
>
>      qemu_opts_del(opts);
>      return 0;
> @@ -564,6 +563,7 @@ static const AIOCBInfo curl_aiocb_info = {
>  static void curl_readv_bh_cb(void *p)
>  {
>      CURLState *state;
> +    int running;
>
>      CURLAIOCB *acb = p;
>      BDRVCURLState *s = acb->common.bs->opaque;
> @@ -612,8 +612,9 @@ static void curl_readv_bh_cb(void *p)
>      curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
>
>      curl_multi_add_handle(s->multi, state->curl);
> -    curl_multi_do(s);
>
> +    /* Tell curl it needs to kick things off */
> +    curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);

curl_multi_socket_action isn't available on the CURL lib used
by CentOS 5.6.  The obvious workaround is to disable CURL
when calling configure.

Thanks,

Laurent

>  }
>
>  static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
> --
> 1.8.3.1
>
>
Kevin Wolf - May 6, 2014, 9:56 a.m.
Am 06.05.2014 um 11:44 hat Laurent Desnogues geschrieben:
> Hello,
> 
> sorry for another late detection of the use of too recent features.
> 
> On Wed, Apr 30, 2014 at 8:24 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> > From: Matthew Booth <mbooth@redhat.com>
> >
> > Remove calls to curl_multi_do where the relevant handles are already
> > registered to the event loop.
> >
> > Ensure that we kick off socket handling with CURL_SOCKET_TIMEOUT after
> > adding a new handle.
> >
> > Signed-off-by: Matthew Booth <mbooth@redhat.com>
> > Tested-by: Richard W.M. Jones <rjones@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/curl.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/curl.c b/block/curl.c
> > index 50bd05f..fd2756e 100644
> > --- a/block/curl.c
> > +++ b/block/curl.c
> > @@ -535,7 +535,6 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
> >      curl_multi_setopt(s->multi, CURLMOPT_TIMERDATA, s);
> >      curl_multi_setopt(s->multi, CURLMOPT_TIMERFUNCTION, curl_timer_cb);
> >  #endif
> > -    curl_multi_do(s);
> >
> >      qemu_opts_del(opts);
> >      return 0;
> > @@ -564,6 +563,7 @@ static const AIOCBInfo curl_aiocb_info = {
> >  static void curl_readv_bh_cb(void *p)
> >  {
> >      CURLState *state;
> > +    int running;
> >
> >      CURLAIOCB *acb = p;
> >      BDRVCURLState *s = acb->common.bs->opaque;
> > @@ -612,8 +612,9 @@ static void curl_readv_bh_cb(void *p)
> >      curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
> >
> >      curl_multi_add_handle(s->multi, state->curl);
> > -    curl_multi_do(s);
> >
> > +    /* Tell curl it needs to kick things off */
> > +    curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
> 
> curl_multi_socket_action isn't available on the CURL lib used
> by CentOS 5.6.  The obvious workaround is to disable CURL
> when calling configure.

Matthew, can you please check whether this call can be avoided, or
otherwise change configure to check the presence of the function?

Kevin
Matthew Booth - May 6, 2014, 12:41 p.m.
On 06/05/14 10:56, Kevin Wolf wrote:
> Am 06.05.2014 um 11:44 hat Laurent Desnogues geschrieben:
>> Hello,
>>
>> sorry for another late detection of the use of too recent features.
>>
>> On Wed, Apr 30, 2014 at 8:24 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> From: Matthew Booth <mbooth@redhat.com>
>>>
>>> Remove calls to curl_multi_do where the relevant handles are already
>>> registered to the event loop.
>>>
>>> Ensure that we kick off socket handling with CURL_SOCKET_TIMEOUT after
>>> adding a new handle.
>>>
>>> Signed-off-by: Matthew Booth <mbooth@redhat.com>
>>> Tested-by: Richard W.M. Jones <rjones@redhat.com>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block/curl.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/curl.c b/block/curl.c
>>> index 50bd05f..fd2756e 100644
>>> --- a/block/curl.c
>>> +++ b/block/curl.c
>>> @@ -535,7 +535,6 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>>>      curl_multi_setopt(s->multi, CURLMOPT_TIMERDATA, s);
>>>      curl_multi_setopt(s->multi, CURLMOPT_TIMERFUNCTION, curl_timer_cb);
>>>  #endif
>>> -    curl_multi_do(s);
>>>
>>>      qemu_opts_del(opts);
>>>      return 0;
>>> @@ -564,6 +563,7 @@ static const AIOCBInfo curl_aiocb_info = {
>>>  static void curl_readv_bh_cb(void *p)
>>>  {
>>>      CURLState *state;
>>> +    int running;
>>>
>>>      CURLAIOCB *acb = p;
>>>      BDRVCURLState *s = acb->common.bs->opaque;
>>> @@ -612,8 +612,9 @@ static void curl_readv_bh_cb(void *p)
>>>      curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
>>>
>>>      curl_multi_add_handle(s->multi, state->curl);
>>> -    curl_multi_do(s);
>>>
>>> +    /* Tell curl it needs to kick things off */
>>> +    curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
>>
>> curl_multi_socket_action isn't available on the CURL lib used
>> by CentOS 5.6.  The obvious workaround is to disable CURL
>> when calling configure.
> 
> Matthew, can you please check whether this call can be avoided, or
> otherwise change configure to check the presence of the function?

I should be able to sort this if I can work out how to add a configure
test. When do you need it by? If I'm too slow it's safe to revert this
change. It's just a cleanup to remove use of a deprecated api.

Matt
Kevin Wolf - May 6, 2014, 1:12 p.m.
Am 06.05.2014 um 14:41 hat Matthew Booth geschrieben:
> On 06/05/14 10:56, Kevin Wolf wrote:
> > Am 06.05.2014 um 11:44 hat Laurent Desnogues geschrieben:
> >> curl_multi_socket_action isn't available on the CURL lib used
> >> by CentOS 5.6.  The obvious workaround is to disable CURL
> >> when calling configure.
> > 
> > Matthew, can you please check whether this call can be avoided, or
> > otherwise change configure to check the presence of the function?
> 
> I should be able to sort this if I can work out how to add a configure
> test. When do you need it by? If I'm too slow it's safe to revert this
> change. It's just a cleanup to remove use of a deprecated api.

We have plenty of time until 2.1, so I don't think that's a problem.

Anyway, if you need to do the configure change, just search for "curl" in
configure. We already probe for some functions, you can just add another
call in the program for the test compile.

Kevin

Patch

diff --git a/block/curl.c b/block/curl.c
index 50bd05f..fd2756e 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -535,7 +535,6 @@  static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     curl_multi_setopt(s->multi, CURLMOPT_TIMERDATA, s);
     curl_multi_setopt(s->multi, CURLMOPT_TIMERFUNCTION, curl_timer_cb);
 #endif
-    curl_multi_do(s);
 
     qemu_opts_del(opts);
     return 0;
@@ -564,6 +563,7 @@  static const AIOCBInfo curl_aiocb_info = {
 static void curl_readv_bh_cb(void *p)
 {
     CURLState *state;
+    int running;
 
     CURLAIOCB *acb = p;
     BDRVCURLState *s = acb->common.bs->opaque;
@@ -612,8 +612,9 @@  static void curl_readv_bh_cb(void *p)
     curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
 
     curl_multi_add_handle(s->multi, state->curl);
-    curl_multi_do(s);
 
+    /* Tell curl it needs to kick things off */
+    curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
 }
 
 static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,