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 <vsement...@virtuozzo.com> >> --- >> 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(&new_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, &new_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 still.) > > Max > -- Best regards, Vladimir