Patchwork [1/5] qcow2: introduce check_refcounts_l1/l2() flags

login
register
mail settings
Submitter Stefan Hajnoczi
Date Feb. 5, 2013, 6:54 p.m.
Message ID <1360090451-26543-2-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/218319/
State New
Headers show

Comments

Stefan Hajnoczi - Feb. 5, 2013, 6:54 p.m.
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 <stefanha@redhat.com>
---
 block/qcow2-refcount.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
Eric Blake - Feb. 5, 2013, 9:04 p.m.
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 <stefanha@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 <eblake@redhat.com>
Stefan Hajnoczi - Feb. 6, 2013, 9:29 a.m.
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 <stefanha@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

Patch

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index bc1784c..e2509ab 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -914,6 +914,11 @@  static void inc_refcounts(BlockDriverState *bs,
     }
 }
 
+/* Flags for check_refcounts_l1() and check_refcounts_l2() */
+enum {
+    CHECK_OFLAG_COPIED = 0x1,   /* check QCOW_OFLAG_COPIED matches refcount */
+};
+
 /*
  * Increases the refcount in the given refcount table for the all clusters
  * referenced in the L2 table. While doing so, performs some checks on L2
@@ -924,7 +929,7 @@  static void inc_refcounts(BlockDriverState *bs,
  */
 static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
     uint16_t *refcount_table, int refcount_table_size, int64_t l2_offset,
-    int check_copied)
+    int flags)
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t *l2_table, l2_entry;
@@ -971,7 +976,7 @@  static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
             /* QCOW_OFLAG_COPIED must be set iff refcount == 1 */
             uint64_t offset = l2_entry & L2E_OFFSET_MASK;
 
-            if (check_copied) {
+            if (flags & CHECK_OFLAG_COPIED) {
                 refcount = get_refcount(bs, offset >> s->cluster_bits);
                 if (refcount < 0) {
                     fprintf(stderr, "Can't get refcount for offset %"
@@ -1028,7 +1033,7 @@  static int check_refcounts_l1(BlockDriverState *bs,
                               uint16_t *refcount_table,
                               int refcount_table_size,
                               int64_t l1_table_offset, int l1_size,
-                              int check_copied)
+                              int flags)
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t *l1_table, l2_offset, l1_size2;
@@ -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);
                 if (refcount < 0) {
@@ -1086,7 +1091,7 @@  static int check_refcounts_l1(BlockDriverState *bs,
 
             /* Process and check L2 entries */
             ret = check_refcounts_l2(bs, res, refcount_table,
-                refcount_table_size, l2_offset, check_copied);
+                refcount_table_size, l2_offset, flags);
             if (ret < 0) {
                 goto fail;
             }
@@ -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);
     if (ret < 0) {
         goto fail;
     }