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

[Eric Blake]

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.

> @@ -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.

But as fixing whitespace doesn't affect semantics, and I assume
checkpatch.pl isn't calling out either case of potentially unusual
spacing, you are free to use this whether or not you make a
reindentation change:

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature