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;
> + }
> +
> + 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
