| Message ID | 1360090451-26543-2-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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; }
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(-)