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,
-
