Re: [Qemu-devel] [PATCH 08/27] block/parallels: _co_writev callback for Parallels format

[Denis V. Lunev]

On 22/04/15 16:08, Stefan Hajnoczi wrote:
> On Wed, Mar 11, 2015 at 01:28:02PM +0300, Denis V. Lunev wrote:
> > +static int64_t allocate_cluster(BlockDriverState *bs, int64_t sector_num)
> > +{
> > +    BDRVParallelsState *s = bs->opaque;
> > +    uint32_t idx, offset, tmp;
> > +    int64_t pos;
> > +    int ret;
> > +
> > +    idx = sector_num / s->tracks;
> > +    offset = sector_num % s->tracks;
> > +
> > +    if (idx >= s->catalog_size) {
> > +        return -EINVAL;
> > +    }
> > +    if (s->catalog_bitmap[idx] != 0) {
> > +        return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
> > +    }
> > +
> > +    pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
> > +    bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
> > +    s->catalog_bitmap[idx] = pos / s->off_multiplier;
> > +
> > +    tmp = cpu_to_le32(s->catalog_bitmap[idx]);
> > +
> > +    ret = bdrv_pwrite_sync(bs->file,
> > +            sizeof(ParallelsHeader) + idx * sizeof(tmp), &tmp, sizeof(tmp));
> What is the purpose of the sync?
This is necessary to preserve image consistency on crash from
my point of view. There is no check consistency at the moment.
The sync will be removed later when proper crash detection
code will be added (patches 19, 20, 21)

On the other hand this _sync  was borrowed from qcow.c:get_cluster_offset
which was used as a base of this work.
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +    return (uint64_t)s->catalog_bitmap[idx] * s->off_multiplier + offset;
> > +}
> This function is missing error handling.  If the catalog bitmap update
> cannot be written to file then our in-memory copy should also be
> reverted back to 0 (unallocated).
ok