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

[Max Reitz]

On 2014-10-22 at 14:21, Gonglei wrote:
> On 2014/10/22 20:02, Max Reitz wrote:
>
> > On 2014-10-22 at 14:01, Max Reitz wrote:
> > > 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...?
> > Oh, but I see something different: qemu_try_blockalign() never returns
> > NULL, not even when you request 0 bytes.
>
> Yes. But if l1_size2 is zero, will waste memory or some other problems.
> Please see below:
>
> the original code:
> l1_table = g_try_malloc0(align_offset(0, 512));
>   -> l1_table = g_try_malloc0(0)
> so  l1_table == NULL.
>
> after this patch:
> l1_table = qemu_try_blockalign(bs->file, 0)
> l1_table will not be NULL.

Okay, so you meant "if l1_size2 == 0 and l1_table != NULL".

> I don't know whether l1_size2 can be zero or not.

Probably not, but if it cannot be zero, testing for that case makes even 
less sense.

Max