Re: [Qemu-devel] [PATCH v2 5/7] block/qcow2-refcount: check_refcounts_l2: split fix_l2_entry_to_zero

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>> ---
>>   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