Re: [PATCH 08/10] qcow2-refcount: improve style of check_refcounts_l1()

[Vladimir Sementsov-Ogievskiy]

04.05.2021 22:53, Eric Blake wrote:
> On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> >   - use g_autofree for l1_table
> >   - better name for size in bytes variable
> >   - reduce code blocks nesting
> >   - whitespaces, braces, newlines
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block/qcow2-refcount.c | 97 +++++++++++++++++++++---------------------
> >   1 file changed, 49 insertions(+), 48 deletions(-)
> >
> > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> > index 44fc0dd5dc..eb6de3dabd 100644
> > --- a/block/qcow2-refcount.c
> > +++ b/block/qcow2-refcount.c
> > @@ -1864,71 +1864,72 @@ static int check_refcounts_l1(BlockDriverState *bs,
> >                                 int flags, BdrvCheckMode fix, bool active)
> >   {
> >       BDRVQcow2State *s = bs->opaque;
> > -    uint64_t *l1_table = NULL, l2_offset, l1_size2;
> > +    size_t l1_size_bytes = l1_size * L1E_SIZE;
> > +    g_autofree uint64_t *l1_table = g_try_malloc(l1_size_bytes);
>
> Note that this now happens...
>
> > +    uint64_t l2_offset;
> >       int i, ret;
> >   
> > -    l1_size2 = l1_size * L1E_SIZE;
> > +    if (!l1_size) {
> > +        return 0;
>
> ...before you validate whether l1_size is non-zero, which can result in
> g_try_malloc(0).  Probably harmless, but it might be better if you declare
>   g_autofree uint64_t *l1_table = NULL;
> and then initialize it via malloc only after the sanity check.

Thinking a bit, another thing I don't like is that check if (l1_table == NULL) 
doesn't immediately follow g_try_malloc() call. So, will move it.

--
Best regards,
Vladimir