Re: [Qemu-devel] [PATCH v5 06/11] curl: introduce CURLDataCache

[Fam Zheng]

On Thu, 05/23 16:09, Stefan Hajnoczi wrote:
> On Thu, May 23, 2013 at 11:38:04AM +0800, Fam Zheng wrote:
> > +typedef struct CURLDataCache {
> > +    char *data;
> > +    size_t base_pos;
> 
> Must be int64_t.  QEMU compiled on 32-bit hosts would only allow 4 GB
> images with size_t!

OK.

> 
> > +    size_t data_len;
> > +    size_t write_pos;
> > +    /* Ref count for CURLState */
> > +    int use_count;
> 
> It's better to introduce this field when you add code to use it.  When
> possible, don't add unused code in a patch, it makes it harder to
> review.

Moving to later patch.

> 
> > +static void curl_complete_io(BDRVCURLState *bs, CURLAIOCB *acb,
> > +                             CURLDataCache *cache)
> > +{
> > +    size_t aio_base = acb->sector_num * SECTOR_SIZE;
> 
> int64_t
> 
> > +    size_t aio_bytes = acb->nb_sectors * SECTOR_SIZE;
> > +    size_t off = aio_base - cache->base_pos;
> > +
> > +    qemu_iovec_from_buf(acb->qiov, 0, cache->data + off, aio_bytes);
> > +    acb->common.cb(acb->common.opaque, 0);
> > +    DPRINTF("AIO Request OK: %10zd %10zd\n", aio_base, aio_bytes);
> 
> PRId64 for 64-bit aio_base

OK, thanks.

> 
> > @@ -589,26 +577,24 @@ static const AIOCBInfo curl_aiocb_info = {
> >  static void curl_readv_bh_cb(void *p)
> >  {
> >      CURLState *state;
> > -
> > +    CURLDataCache *cache = NULL;
> >      CURLAIOCB *acb = p;
> >      BDRVCURLState *s = acb->common.bs->opaque;
> > +    size_t aio_base, aio_bytes;
> 
> int64_t aio_base;

Yes, will change.

-- 
Fam