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

[Fam Zheng]

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 <address@hidden>
> Cc: Fam Zheng <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>

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