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 <stefa...@redhat.com>
> ---
> 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 <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
