From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59862) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WhHRX-0000FU-4r for qemu-devel@nongnu.org; Mon, 05 May 2014 07:52:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WhHRR-0001y8-Dq for qemu-devel@nongnu.org; Mon, 05 May 2014 07:52:23 -0400 Received: from mail-we0-x230.google.com ([2a00:1450:400c:c03::230]:45900) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WhHRR-0001xt-29 for qemu-devel@nongnu.org; Mon, 05 May 2014 07:52:17 -0400 Received: by mail-we0-f176.google.com with SMTP id q59so5026849wes.35 for ; Mon, 05 May 2014 04:52:15 -0700 (PDT) Date: Mon, 5 May 2014 13:52:12 +0200 From: Stefan Hajnoczi Message-ID: <20140505115212.GA16173@stefanha-thinkpad.redhat.com> References: <1398956086-20171-1-git-send-email-stefanha@redhat.com> <1398956086-20171-7-git-send-email-stefanha@redhat.com> <20140504110026.GH1124@T430.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140504110026.GH1124@T430.nay.redhat.com> Subject: Re: [Qemu-devel] [PATCH 06/22] curl: implement .bdrv_detach/attach_aio_context() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Kevin Wolf , "Shergill, Gurinder" , qemu-devel@nongnu.org, Alexander Graf , Stefan Hajnoczi , Paolo Bonzini , "Vinod, Chegu" 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 > > Cc: Fam Zheng > > Signed-off-by: Stefan Hajnoczi > > 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