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. :)
> +
> + 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.
> + 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.
Thanks,
Fam
