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 <ag...@suse.de>
> > Cc: Fam Zheng <f...@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefa...@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
