[4/7] curl: split curl_find_state/curl_init_state

If curl_easy_init fails, a CURLState is left with s->in_use = 1.  Split
curl_init_state in two, so that we can distinguish the two failures and
call curl_clean_state if needed.

While at it, simplify curl_find_state, removing a dummy loop.  The
aio_poll loop is moved to the sole caller that needs it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/curl.c | 52 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

On 10.05.2017 16:32, Paolo Bonzini wrote:
> If curl_easy_init fails, a CURLState is left with s->in_use = 1.  Split
> curl_init_state in two, so that we can distinguish the two failures and
> call curl_clean_state if needed.
> 
> While at it, simplify curl_find_state, removing a dummy loop.  The
> aio_poll loop is moved to the sole caller that needs it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/curl.c | 52 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 22 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index b18e79bf54..4b4d5a2389 100644
> --- a/block/curl.c
> +++ b/block/curl.c

[...]

> @@ -856,8 +854,18 @@ static void curl_readv_bh_cb(void *p)
>      }
>  
>      // No cache found, so let's start a new request
> -    state = curl_init_state(acb->common.bs, s);
> -    if (!state) {
> +    for (;;) {
> +        state = curl_find_state(s);
> +        if (state) {
> +            break;
> +        }
> +        qemu_mutex_unlock(&s->mutex);
> +        aio_poll(bdrv_get_aio_context(bs), true);
> +        qemu_mutex_lock(&s->mutex);
> +    }
> +
> +    if (curl_init_state(s, state) < 0) {
> +        curl_clean_state(state);

We could also initialize state to NULL and call curl_clean_state() under
out: if state != NULL. Then again, it would only save us two
curl_clean_state() instances...

So I'll leave it to you and unconditionally give a

Reviewed-by: Max Reitz <mreitz@redhat.com>

>          ret = -EIO;
>          goto out;
>      }
>

On 10/05/2017 19:26, Max Reitz wrote:
>> +    if (curl_init_state(s, state) < 0) {
>> +        curl_clean_state(state);
>
> We could also initialize state to NULL and call curl_clean_state() under
> out: if state != NULL. Then again, it would only save us two
> curl_clean_state() instances...
> 
> So I'll leave it to you and unconditionally give a
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Makes sense, but OTOH you could also move locking to curl_co_preadv and
get rid of "out" altogether.  So for now I left curl_clean_state here.

Paolo

On Wed, May 10, 2017 at 04:32:02PM +0200, Paolo Bonzini wrote:
> If curl_easy_init fails, a CURLState is left with s->in_use = 1.  Split
> curl_init_state in two, so that we can distinguish the two failures and
> call curl_clean_state if needed.
> 
> While at it, simplify curl_find_state, removing a dummy loop.  The
> aio_poll loop is moved to the sole caller that needs it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/curl.c | 52 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 22 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index b18e79bf54..4b4d5a2389 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -455,34 +455,27 @@ static void curl_multi_timeout_do(void *arg)
>  }
>  
>  /* Called with s->mutex held.  */
> -static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
> +static CURLState *curl_find_state(BDRVCURLState *s)
>  {
>      CURLState *state = NULL;
> -    int i, j;
> -
> -    do {
> -        for (i=0; i<CURL_NUM_STATES; i++) {
> -            for (j=0; j<CURL_NUM_ACB; j++)
> -                if (s->states[i].acb[j])
> -                    continue;
> -            if (s->states[i].in_use)
> -                continue;
> +    int i;
>  
> +    for (i=0; i<CURL_NUM_STATES; i++) {
> +        if (!s->states[i].in_use) {
>              state = &s->states[i];
>              state->in_use = 1;
>              break;
>          }
> -        if (!state) {
> -            qemu_mutex_unlock(&s->mutex);
> -            aio_poll(bdrv_get_aio_context(bs), true);
> -            qemu_mutex_lock(&s->mutex);
> -        }
> -    } while(!state);
> +    }
> +    return state;
> +}
>  
> +static int curl_init_state(BDRVCURLState *s, CURLState *state)
> +{
>      if (!state->curl) {
>          state->curl = curl_easy_init();
>          if (!state->curl) {
> -            return NULL;
> +            return -EIO;
>          }
>          curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
>          curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
> @@ -535,7 +528,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
>      QLIST_INIT(&state->sockets);
>      state->s = s;
>  
> -    return state;
> +    return 0;
>  }
>  
>  /* Called with s->mutex held.  */
> @@ -756,13 +749,18 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>      s->aio_context = bdrv_get_aio_context(bs);
>      s->url = g_strdup(file);
>      qemu_mutex_lock(&s->mutex);
> -    state = curl_init_state(bs, s);
> +    state = curl_find_state(s);
>      qemu_mutex_unlock(&s->mutex);
> -    if (!state)
> +    if (!state) {
>          goto out_noclean;
> +    }
>  
>      // Get file size
>  
> +    if (curl_init_state(s, state) < 0) {
> +        goto out;
> +    }
> +
>      s->accept_range = false;
>      curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
>      curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
> @@ -856,8 +854,18 @@ static void curl_readv_bh_cb(void *p)
>      }
>  
>      // No cache found, so let's start a new request
> -    state = curl_init_state(acb->common.bs, s);
> -    if (!state) {
> +    for (;;) {
> +        state = curl_find_state(s);
> +        if (state) {
> +            break;
> +        }
> +        qemu_mutex_unlock(&s->mutex);
> +        aio_poll(bdrv_get_aio_context(bs), true);
> +        qemu_mutex_lock(&s->mutex);
> +    }
> +
> +    if (curl_init_state(s, state) < 0) {
> +        curl_clean_state(state);

For some reason, I initially thought this might cause problems with the
assert in curl_clean_state(), but that isn't the case.

Reviewed-by: Jeff Cody <jcody@redhat.com>

>          ret = -EIO;
>          goto out;
>      }
> -- 
> 2.12.2
> 
>