Re: [Qemu-devel] [PATCH v5 07/11] curl: make use of CURLDataCache.

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, May 23, 2013 at 11:38:05AM +0800, Fam Zheng wrote:
> @@ -221,31 +215,35 @@ static void curl_complete_io(BDRVCURLState *bs, CURLAIOCB *acb,
>  
>  static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>  {
> -    CURLState *s = ((CURLState*)opaque);
> +    CURLState *s = (CURLState *)opaque;

You can drop the cast completely because opaque is void*.

> +    CURLDataCache *c = s->cache;
>      size_t realsize = size * nmemb;
> -    int i;
> -
> -    DPRINTF("CURL: Just reading %zd bytes\n", realsize);
> +    CURLAIOCB *acb;
>  
> -    if (!s || !s->orig_buf)
> +    if (!c || !c->data) {
>          goto read_end;
> +    }
> +    if (c->write_pos >= c->data_len) {
> +        goto read_end;
> +    }
> +    memcpy(c->data + c->write_pos, ptr,
> +           MIN(realsize, c->data_len - c->write_pos));
> +    c->write_pos += realsize;
> +    if (c->write_pos >= c->data_len) {
> +        c->write_pos = c->data_len;
> +    }
>  
> -    memcpy(s->orig_buf + s->buf_off, ptr, realsize);
> -    s->buf_off += realsize;

Why did you add MIN(realsize, c->data_len - c->write_pos)?  The original
code trusts realsize to be within s->orig_buf.

> -
> -    for(i=0; i<CURL_NUM_ACB; i++) {
> -        CURLAIOCB *acb = s->acb[i];
> -
> -        if (!acb)
> -            continue;
> -
> -        if ((s->buf_off >= acb->end)) {
> -            qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
> -                                acb->end - acb->start);
> -            acb->common.cb(acb->common.opaque, 0);
> -            qemu_aio_release(acb);
> -            s->acb[i] = NULL;
> +    acb = QLIST_FIRST(&s->s->acbs);
> +    while (acb) {
> +        size_t aio_base = acb->sector_num * SECTOR_SIZE;

int64_t

> @@ -600,29 +596,41 @@ static void curl_readv_bh_cb(void *p)
>      // No cache found, so let's start a new request
>      state = curl_init_state(s);
>      if (!state) {
> -        acb->common.cb(acb->common.opaque, -EIO);
> -        qemu_aio_release(acb);
> -        return;
> +        goto err_release;
>      }
>  
> -    acb->start = 0;
> -    acb->end = (acb->nb_sectors * SECTOR_SIZE);
> -
> -    state->buf_off = 0;
> -    if (state->orig_buf)
> -        g_free(state->orig_buf);
> -    state->buf_start = start;
> -    state->buf_len = acb->end + s->readahead_size;
> -    end = MIN(start + state->buf_len, s->len) - 1;
> -    state->orig_buf = g_malloc(state->buf_len);
> -    state->acb[0] = acb;
> -
> -    snprintf(state->range, sizeof(state->range) - 1, "%zd-%zd", start, end);
> -    DPRINTF("CURL (AIO): Reading %d at %zd (%s)\n",
> -            (acb->nb_sectors * SECTOR_SIZE), start, state->range);
> -    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
> +    cache = g_malloc0(sizeof(CURLDataCache));
> +    cache->base_pos = acb->sector_num * SECTOR_SIZE;
> +    cache->data_len = aio_bytes + s->readahead_size;
> +    cache->write_pos = 0;
> +    cache->data = g_malloc(cache->data_len);
>  
> +    QLIST_INSERT_HEAD(&s->acbs, acb, next);
> +    snprintf(state->range, sizeof(state->range) - 1, "%zd-%zd", cache->base_pos,
> +             cache->base_pos + cache->data_len);
> +    DPRINTF("Reading range: %s\n", state->range);
> +    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
> +    QLIST_INSERT_HEAD(&s->cache, cache, next);
> +    state->cache = cache;
> +    cache->use_count++;

I don't see where you bump the use_count when a cache lookup is
successful.  Maybe I just missed it in the other patches.