Re: [Qemu-devel] [PATCH v2 5/7] block/qcow2-refcount: check_refcounts_l2: split fix_l2_entry_to_zero
08.10.2018 22:54, Max Reitz wrote: > On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: >> Split entry repairing to separate function, to be reused later. >> >> Note: entry in in-memory l2 table (local variable in >> check_refcounts_l2) is not updated after this patch. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> block/qcow2-refcount.c | 147 >> - >> 1 file changed, 109 insertions(+), 38 deletions(-) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 3b057b635d..d9c8cd666b 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1554,6 +1554,99 @@ enum { >> CHECK_FRAG_INFO = 0x2, /* update BlockFragInfo counters */ >> }; >> >> +/* Update entry in L1 or L2 table >> + * >> + * Returns: -errno if overlap check failed >> + * 0 if write failed >> + * 1 on success >> + */ >> +static int write_table_entry(BlockDriverState *bs, const char *table_name, >> + uint64_t table_offset, int entry_index, >> + uint64_t new_val, int ign) >> +{ >> +int ret; >> +uint64_t entry_offset = >> +table_offset + (uint64_t)entry_index * sizeof(new_val); >> + >> +cpu_to_be64s(_val); >> +ret = qcow2_pre_write_overlap_check(bs, ign, entry_offset, >> sizeof(new_val)); >> +if (ret < 0) { >> +fprintf(stderr, >> +"ERROR: Can't write %s table entry: overlap check failed: >> %s\n", > I recently complained to Berto that I don't like elisions ("can't") in > user interfaces, so I suppose I'll have to complain here, too, that I'd > prefer a "Cannot". > >> +table_name, strerror(-ret)); >> +return ret; >> +} >> + >> +ret = bdrv_pwrite_sync(bs->file, entry_offset, _val, >> sizeof(new_val)); >> +if (ret < 0) { >> +fprintf(stderr, "ERROR: Failed to overwrite %s table entry: %s\n", >> +table_name, strerror(-ret)); >> +return 0; >> +} >> + >> +return 1; >> +} >> + >> +/* Try to fix (if allowed) entry in L1 or L2 table. Update @res >> correspondingly. >> + * >> + * Returns: -errno if overlap check failed >> + * 0 if entry was not updated for other reason >> + *(fixing disabled or write failed) >> + * 1 on success >> + */ >> +static int fix_table_entry(BlockDriverState *bs, BdrvCheckResult *res, >> + BdrvCheckMode fix, const char *table_name, >> + uint64_t table_offset, int entry_index, >> + uint64_t new_val, int ign, >> + const char *fmt, va_list args) >> +{ >> +int ret; >> + >> +fprintf(stderr, fix & BDRV_FIX_ERRORS ? "Repairing: " : "ERROR: "); >> +vfprintf(stderr, fmt, args); >> +fprintf(stderr, "\n"); > If you're just going to print this here, right at the start, I think it > would be better to just do it in the caller. > > Sure, with this solution the caller safes an fprintf() call, but I find > it a bit over the top to start with vararg handling here when the caller > can just do an additional fprintf(). > > (Also, I actually find it clearer if you have to call two separate > functions.) > >> + >> +if (!(fix & BDRV_FIX_ERRORS)) { >> +res->corruptions++; >> +return 0; >> +} hmm, after dropping printfs, this if becomes strange too. it's the only use of "fix", and not correspond to function name (correspond to comments, but it's a bad reason). >> + >> +ret = write_table_entry(bs, table_name, table_offset, entry_index, >> new_val, >> +ign); >> + >> +if (ret == 1) { >> +res->corruptions_fixed++; >> +} else { >> +res->check_errors++; >> +} >> + >> +return ret; >> +} >> + >> +/* Make L2 entry to be QCOW2_CLUSTER_ZERO_PLAIN >> + * >> + * Returns: -errno if overlap check failed >> + * 0 if write failed >> + * 1 on success >> + */ >> +static int fix_l2_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res, >> +BdrvCheckMode fix, int64_t l2_offset, >> +int l2_index, bool active, >> +const char *fmt, ...) >> +{ >> +int ret; >> +int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2; >> +uint64_t l2_entry = QCOW_OFLAG_ZERO; >> +va_list args; >> + >> +va_start(args, fmt); >> +ret = fix_table_entry(bs, res, fix, "L2", l2_offset, l2_index, l2_entry, >> + ign, fmt, args); >> +va_end(args); >> + >> +return ret; >> +} > If you drop the fprintf() from fix_table_entry(), this function will > make less sense as well. Just calling fix_table_entry() directly will > be just as easy. > > (Yes, I see that you use the function in patch 7 again, and yes, you'd > have to use a full line for the "active ?:" ternary, but
Re: [Qemu-devel] [PATCH v2 5/7] block/qcow2-refcount: check_refcounts_l2: split fix_l2_entry_to_zero
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: > Split entry repairing to separate function, to be reused later. > > Note: entry in in-memory l2 table (local variable in > check_refcounts_l2) is not updated after this patch. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-refcount.c | 147 > - > 1 file changed, 109 insertions(+), 38 deletions(-) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 3b057b635d..d9c8cd666b 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1554,6 +1554,99 @@ enum { > CHECK_FRAG_INFO = 0x2, /* update BlockFragInfo counters */ > }; > > +/* Update entry in L1 or L2 table > + * > + * Returns: -errno if overlap check failed > + * 0 if write failed > + * 1 on success > + */ > +static int write_table_entry(BlockDriverState *bs, const char *table_name, > + uint64_t table_offset, int entry_index, > + uint64_t new_val, int ign) > +{ > +int ret; > +uint64_t entry_offset = > +table_offset + (uint64_t)entry_index * sizeof(new_val); > + > +cpu_to_be64s(_val); > +ret = qcow2_pre_write_overlap_check(bs, ign, entry_offset, > sizeof(new_val)); > +if (ret < 0) { > +fprintf(stderr, > +"ERROR: Can't write %s table entry: overlap check failed: > %s\n", I recently complained to Berto that I don't like elisions ("can't") in user interfaces, so I suppose I'll have to complain here, too, that I'd prefer a "Cannot". > +table_name, strerror(-ret)); > +return ret; > +} > + > +ret = bdrv_pwrite_sync(bs->file, entry_offset, _val, > sizeof(new_val)); > +if (ret < 0) { > +fprintf(stderr, "ERROR: Failed to overwrite %s table entry: %s\n", > +table_name, strerror(-ret)); > +return 0; > +} > + > +return 1; > +} > + > +/* Try to fix (if allowed) entry in L1 or L2 table. Update @res > correspondingly. > + * > + * Returns: -errno if overlap check failed > + * 0 if entry was not updated for other reason > + *(fixing disabled or write failed) > + * 1 on success > + */ > +static int fix_table_entry(BlockDriverState *bs, BdrvCheckResult *res, > + BdrvCheckMode fix, const char *table_name, > + uint64_t table_offset, int entry_index, > + uint64_t new_val, int ign, > + const char *fmt, va_list args) > +{ > +int ret; > + > +fprintf(stderr, fix & BDRV_FIX_ERRORS ? "Repairing: " : "ERROR: "); > +vfprintf(stderr, fmt, args); > +fprintf(stderr, "\n"); If you're just going to print this here, right at the start, I think it would be better to just do it in the caller. Sure, with this solution the caller safes an fprintf() call, but I find it a bit over the top to start with vararg handling here when the caller can just do an additional fprintf(). (Also, I actually find it clearer if you have to call two separate functions.) > + > +if (!(fix & BDRV_FIX_ERRORS)) { > +res->corruptions++; > +return 0; > +} > + > +ret = write_table_entry(bs, table_name, table_offset, entry_index, > new_val, > +ign); > + > +if (ret == 1) { > +res->corruptions_fixed++; > +} else { > +res->check_errors++; > +} > + > +return ret; > +} > + > +/* Make L2 entry to be QCOW2_CLUSTER_ZERO_PLAIN > + * > + * Returns: -errno if overlap check failed > + * 0 if write failed > + * 1 on success > + */ > +static int fix_l2_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res, > +BdrvCheckMode fix, int64_t l2_offset, > +int l2_index, bool active, > +const char *fmt, ...) > +{ > +int ret; > +int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2; > +uint64_t l2_entry = QCOW_OFLAG_ZERO; > +va_list args; > + > +va_start(args, fmt); > +ret = fix_table_entry(bs, res, fix, "L2", l2_offset, l2_index, l2_entry, > + ign, fmt, args); > +va_end(args); > + > +return ret; > +} If you drop the fprintf() from fix_table_entry(), this function will make less sense as well. Just calling fix_table_entry() directly will be just as easy. (Yes, I see that you use the function in patch 7 again, and yes, you'd have to use a full line for the "active ?:" ternary, but still.) Max signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2 5/7] block/qcow2-refcount: check_refcounts_l2: split fix_l2_entry_to_zero
Split entry repairing to separate function, to be reused later. Note: entry in in-memory l2 table (local variable in check_refcounts_l2) is not updated after this patch. Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/qcow2-refcount.c | 147 - 1 file changed, 109 insertions(+), 38 deletions(-) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 3b057b635d..d9c8cd666b 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1554,6 +1554,99 @@ enum { CHECK_FRAG_INFO = 0x2, /* update BlockFragInfo counters */ }; +/* Update entry in L1 or L2 table + * + * Returns: -errno if overlap check failed + * 0 if write failed + * 1 on success + */ +static int write_table_entry(BlockDriverState *bs, const char *table_name, + uint64_t table_offset, int entry_index, + uint64_t new_val, int ign) +{ +int ret; +uint64_t entry_offset = +table_offset + (uint64_t)entry_index * sizeof(new_val); + +cpu_to_be64s(_val); +ret = qcow2_pre_write_overlap_check(bs, ign, entry_offset, sizeof(new_val)); +if (ret < 0) { +fprintf(stderr, +"ERROR: Can't write %s table entry: overlap check failed: %s\n", +table_name, strerror(-ret)); +return ret; +} + +ret = bdrv_pwrite_sync(bs->file, entry_offset, _val, sizeof(new_val)); +if (ret < 0) { +fprintf(stderr, "ERROR: Failed to overwrite %s table entry: %s\n", +table_name, strerror(-ret)); +return 0; +} + +return 1; +} + +/* Try to fix (if allowed) entry in L1 or L2 table. Update @res correspondingly. + * + * Returns: -errno if overlap check failed + * 0 if entry was not updated for other reason + *(fixing disabled or write failed) + * 1 on success + */ +static int fix_table_entry(BlockDriverState *bs, BdrvCheckResult *res, + BdrvCheckMode fix, const char *table_name, + uint64_t table_offset, int entry_index, + uint64_t new_val, int ign, + const char *fmt, va_list args) +{ +int ret; + +fprintf(stderr, fix & BDRV_FIX_ERRORS ? "Repairing: " : "ERROR: "); +vfprintf(stderr, fmt, args); +fprintf(stderr, "\n"); + +if (!(fix & BDRV_FIX_ERRORS)) { +res->corruptions++; +return 0; +} + +ret = write_table_entry(bs, table_name, table_offset, entry_index, new_val, +ign); + +if (ret == 1) { +res->corruptions_fixed++; +} else { +res->check_errors++; +} + +return ret; +} + +/* Make L2 entry to be QCOW2_CLUSTER_ZERO_PLAIN + * + * Returns: -errno if overlap check failed + * 0 if write failed + * 1 on success + */ +static int fix_l2_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res, +BdrvCheckMode fix, int64_t l2_offset, +int l2_index, bool active, +const char *fmt, ...) +{ +int ret; +int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2; +uint64_t l2_entry = QCOW_OFLAG_ZERO; +va_list args; + +va_start(args, fmt); +ret = fix_table_entry(bs, res, fix, "L2", l2_offset, l2_index, l2_entry, + ign, fmt, args); +va_end(args); + +return ret; +} + /* * 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 @@ -1646,46 +1739,24 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, if (qcow2_get_cluster_type(l2_entry) == QCOW2_CLUSTER_ZERO_ALLOC) { -fprintf(stderr, "%s offset=%" PRIx64 ": Preallocated zero " -"cluster is not properly aligned; L2 entry " -"corrupted.\n", -fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", +ret = fix_l2_entry_to_zero( +bs, res, fix, l2_offset, i, active, +"offset=%" PRIx64 ": Preallocated zero cluster is " +"not properly aligned; L2 entry corrupted.", offset); -if (fix & BDRV_FIX_ERRORS) { -uint64_t l2e_offset = -l2_offset + (uint64_t)i * sizeof(uint64_t); -int ign = active ? QCOW2_OL_ACTIVE_L2 : - QCOW2_OL_INACTIVE_L2; - -l2_entry = QCOW_OFLAG_ZERO; -l2_table[i] = cpu_to_be64(l2_entry); -ret = qcow2_pre_write_overlap_check(bs, ign, -