Re: [Qemu-devel] [PATCH 1/5] qcow2: introduce check_refcounts_l1/l2() flags

[Stefan Hajnoczi]

On Tue, Feb 05, 2013 at 02:04:42PM -0700, Eric Blake wrote:
> On 02/05/2013 11:54 AM, Stefan Hajnoczi wrote:
> > The check_refcounts_l1/l2() functions have a check_copied argument to
> > check that the QCOW_O_COPIED flag is consistent with refcount == 1.
> > This should be a bool, not an int.
> > 
> > However, the next patch introduces qcow2 fragmentation statistics and
> > also needs to pass an option to check_refcounts_l1/l2().  This is a good
> > opportunity to use an int flags field.
> > 
> > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > ---
> >  block/qcow2-refcount.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > @@ -1057,7 +1062,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
> >          l2_offset = l1_table[i];
> >          if (l2_offset) {
> >              /* QCOW_OFLAG_COPIED must be set iff refcount == 1 */
> > -            if (check_copied) {
> > +            if (flags & CHECK_OFLAG_COPIED) {
> >                  refcount = get_refcount(bs, (l2_offset & 
> > ~QCOW_OFLAG_COPIED)
> >                      >> s->cluster_bits);
> 
> Here, I'm not sure if indentation is off; 'git grep -B1 "    >>"' didn't
> make it very obvious if it is more common to indent the operator to the
> level of the function call '(' instead of just four spaces, when
> splitting a shift expression as part of a larger assignment statement.
> Personally, I prefer the style:
> 
>     refcount = get_refcount(bs, ((l2_offset & ~QCOW_OFLAG_COPIED)
>                                  >> s->cluster_bits));
> 
> that is, using another layer of () to make it obvious why the >>
> operator is being further indented.  But I don't think my personal style
> has any mandate in HACKING; and at any rate, this problem is
> pre-existing and wasn't touched by your patch.

Not changing this since it's not touched by the patch.

> > @@ -1128,7 +1133,8 @@ int qcow2_check_refcounts(BlockDriverState *bs, 
> > BdrvCheckResult *res,
> >  
> >      /* current L1 table */
> >      ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
> > -                       s->l1_table_offset, s->l1_size, 1);
> > +                       s->l1_table_offset, s->l1_size,
> > +                       CHECK_OFLAG_COPIED);
> 
> Here, the indentation is definitely off, as a pre-existing problem, but
> definitely touched by you, so I would suggest fixing it.

Fixed in the next version and another similar instance.

Stefan