Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Max Reitz]

On 2014-10-22 at 13:59, Gonglei wrote:
> On 2014/10/22 19:45, Zhang Haoyu wrote:
>
> > Use local variable to bdrv_pwrite_sync L1 table,
> > needless to make conversion of cached L1 table between
> > big-endian and host style.
> >
> > Signed-off-by: Zhang Haoyu <address@hidden>
> > Reviewed-by: Max Reitz <address@hidden>
> > ---
> > v3 -> v4:
> >   - convert local L1 table to host-style before copy it
> >     back to s->l1_table
> >
> > v2 -> v3:
> >   - replace g_try_malloc0 with qemu_try_blockalign
> >   - copy the latest local L1 table back to s->l1_table
> >     after successfully bdrv_pwrite_sync L1 table
> >
> > v1 -> v2:
> >   - remove the superflous assignment, l1_table = NULL;
> >   - replace 512 with BDRV_SECTOR_SIZE, and align_offset with ROUND_UP
> >   - remove needless check of if (l1_table) before g_free(l1_table)
> >
> >   block/qcow2-refcount.c | 28 ++++++++++++----------------
> >   1 file changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> > index 2bcaaf9..3e4050a 100644
> > --- a/block/qcow2-refcount.c
> > +++ b/block/qcow2-refcount.c
> > @@ -881,14 +881,17 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
> >   {
> >       BDRVQcowState *s = bs->opaque;
> >       uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2;
> > -    bool l1_allocated = false;
> >       int64_t old_offset, old_l2_offset;
> >       int i, j, l1_modified = 0, nb_csectors, refcount;
> >       int ret;
> >   
> >       l2_table = NULL;
> > -    l1_table = NULL;
> >       l1_size2 = l1_size * sizeof(uint64_t);
> > +    l1_table = qemu_try_blockalign(bs->file, l1_size2);
> > +    if (l1_size2 && l1_table == NULL) {
> I think this check has a logic problem.  If l1_size2 != 0 and l1_table == NULL,
> What will happen?

Then this condition is met and you return with -ENOMEM...?

Max

> Best regards,
> -Gonglei
>
> > +        ret = -ENOMEM;
> > +        goto fail;
> > +    }
> >   
> >       s->cache_discards = true;
> >   
> > @@ -896,13 +899,6 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
> >        * l1_table_offset when it is the current s->l1_table_offset! Be careful
> >        * when changing this! */
> >       if (l1_table_offset != s->l1_table_offset) {
> > -        l1_table = g_try_malloc0(align_offset(l1_size2, 512));
> > -        if (l1_size2 && l1_table == NULL) {
> > -            ret = -ENOMEM;
> > -            goto fail;
> > -        }
> > -        l1_allocated = true;
> > -
> >           ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2);
> >           if (ret < 0) {
> >               goto fail;
> > @@ -912,8 +908,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
> >               be64_to_cpus(&l1_table[i]);
> >       } else {
> >           assert(l1_size == s->l1_size);
> > -        l1_table = s->l1_table;
> > -        l1_allocated = false;
> > +        memcpy(l1_table, s->l1_table, l1_size2);
> >       }
> >   
> >       for(i = 0; i < l1_size; i++) {
> > @@ -1055,13 +1050,14 @@ fail:
> >           }
> >   
> >           ret = bdrv_pwrite_sync(bs->file, l1_table_offset, l1_table, l1_size2);
> > -
> > -        for (i = 0; i < l1_size; i++) {
> > -            be64_to_cpus(&l1_table[i]);
> > +        if (ret == 0) {
> > +            for (i = 0; i < l1_size; i++) {
> > +                be64_to_cpus(&l1_table[i]);
> > +            }
> > +            memcpy(s->l1_table, l1_table, l1_size2);
> >           }
> >       }
> > -    if (l1_allocated)
> > -        g_free(l1_table);
> > +    g_free(l1_table);
> >       return ret;
> >   }
> >