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