Re: [Qemu-devel] [PATCH 06/22] curl: implement .bdrv_detach/attach_aio_context()

[Stefan Hajnoczi <stefanha@gmail.com>]

On Sun, May 04, 2014 at 07:00:26PM +0800, Fam Zheng wrote:
> On Thu, 05/01 16:54, Stefan Hajnoczi wrote:
> > The curl block driver uses fd handlers, timers, and BHs.  The fd
> > handlers and timers are managed on behalf of libcurl, which controls
> > them using callback functions that the block driver implements.
> > 
> > The simplest way to implement .bdrv_detach/attach_aio_context() is to
> > clean up libcurl in the old event loop and initialize it again in the
> > new event loop.  We do not need to keep track of anything since there
> > are no pending requests when the AioContext is changed.
> > 
> > Also make sure to use aio_set_fd_handler() instead of
> > qemu_aio_set_fd_handler() and aio_bh_new() instead of qemu_bh_new() so
> > the current AioContext is passed in.
> > 
> > Cc: Alexander Graf <agraf@suse.de>
> > Cc: Fam Zheng <famz@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Might need to rebase on current master because of the latest curl fixes.
> 
> The patch itself looks good. Minor comments below.
> 
> > ---
> >  block/curl.c | 194 +++++++++++++++++++++++++++++++++++------------------------
> >  1 file changed, 116 insertions(+), 78 deletions(-)
> > 
> > diff --git a/block/curl.c b/block/curl.c
> > index 6731d28..88638ec 100644
> > --- a/block/curl.c
> > +++ b/block/curl.c
> > @@ -430,6 +434,55 @@ static void curl_parse_filename(const char *filename, QDict *options,
> >      g_free(file);
> >  }
> >  
> > +static void curl_detach_aio_context(BlockDriverState *bs)
> > +{
> > +    BDRVCURLState *s = bs->opaque;
> > +    int i;
> > +
> > +    for (i = 0; i < CURL_NUM_STATES; i++) {
> > +        if (s->states[i].in_use) {
> > +            curl_clean_state(&s->states[i]);
> > +        }
> > +        if (s->states[i].curl) {
> > +            curl_easy_cleanup(s->states[i].curl);
> > +            s->states[i].curl = NULL;
> > +        }
> > +        if (s->states[i].orig_buf) {
> > +            g_free(s->states[i].orig_buf);
> > +            s->states[i].orig_buf = NULL;
> > +        }
> > +    }
> > +    if (s->multi) {
> > +        curl_multi_cleanup(s->multi);
> > +        s->multi = NULL;
> > +    }
> > +
> > +    timer_del(&s->timer);
> > +}
> > +
> > +static void curl_attach_aio_context(BlockDriverState *bs,
> > +                                    AioContext *new_context)
> > +{
> > +    BDRVCURLState *s = bs->opaque;
> > +
> > +    aio_timer_init(new_context, &s->timer,
> > +                   QEMU_CLOCK_REALTIME, SCALE_NS,
> > +                   curl_multi_timeout_do, s);
> > +
> > +    // Now we know the file exists and its size, so let's
> > +    // initialize the multi interface!
> 
> I would keep this comment where it was. :)

Good point.

> > +
> > +    s->multi = curl_multi_init();
> 
> Should we assert bdrv_attach_aio_context() is never called repeatedly or
> without a preceding bdrv_detach_aio_context()? Otherwise s->multi could leak.

I'll add the appropriate assertions.

> > +    s->aio_context = new_context;
> > +    curl_multi_setopt(s->multi, CURLMOPT_SOCKETDATA, s);
> > +    curl_multi_setopt(s->multi, CURLMOPT_SOCKETFUNCTION, curl_sock_cb);
> > +#ifdef NEED_CURL_TIMER_CALLBACK
> > +    curl_multi_setopt(s->multi, CURLMOPT_TIMERDATA, s);
> > +    curl_multi_setopt(s->multi, CURLMOPT_TIMERFUNCTION, curl_timer_cb);
> > +#endif
> > +    curl_multi_do(s);
> 
> If you rebase to master, this call to curl_multi_do() is gone, among other
> changes.

Okay.  I'll rebase and resolve the conflicts.

Stefan