Re: [Qemu-devel] [PATCH 4/4] qcow2: Check L1/L2/reftable entries for alignment

[Kevin Wolf]

Am 20.08.2014 um 21:26 hat Max Reitz geschrieben:
> On 20.08.2014 12:51, Kevin Wolf wrote:
> >Am 16.08.2014 um 23:16 hat Max Reitz geschrieben:
> >>Offsets taken from the L1, L2 and refcount tables are generally assumed
> >>to be correctly aligned. However, this cannot be guaranteed if the image
> >>has been written to by something different than qemu, thus check all
> >>offsets taken from these tables for correct cluster alignment.
> >>
> >>Signed-off-by: Max Reitz <address@hidden>
> >>---
> >>  block/qcow2-cluster.c  | 27 ++++++++++++++++++++++++++-
> >>  block/qcow2-refcount.c | 36 ++++++++++++++++++++++++++++++++++--
> >>  2 files changed, 60 insertions(+), 3 deletions(-)

> >>@@ -836,8 +848,13 @@ void qcow2_free_any_clusters(BlockDriverState *bs, 
> >>uint64_t l2_entry,
> >>      case QCOW2_CLUSTER_NORMAL:
> >>      case QCOW2_CLUSTER_ZERO:
> >>          if (l2_entry & L2E_OFFSET_MASK) {
> >>-            qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK,
> >>-                                nb_clusters << s->cluster_bits, type);
> >>+            if (offset_into_cluster(s, l2_entry & L2E_OFFSET_MASK)) {
> >>+                fprintf(stderr, "qcow2: Cannot free unaligned cluster 
> >>%#llx\n",
> >>+                        l2_entry & L2E_OFFSET_MASK);
> >>+            } else {
> >>+                qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK,
> >>+                                    nb_clusters << s->cluster_bits, type);
> >>+            }
> >Hm... Why isn't this a corruption like any other? Unconditional
> >fprintf() is something I don't like a lot.
> 
> We already do it in qcow2_free_clusters().
> 
> I decided not to make it a corruption because we don't lose
> anything. The entry is corrupted, but we don't need it anymore
> anyway; it's overwritten with 0 and wherever the cluster might have
> been meant to be located, it doesn't matter anymore.

I can see your point. This is a tough one: On the one hand, it is
undoubtedly corruption, and usually there is not just one corrupted
entry, so you want the user to check the image. On the other hand, yes,
this is merely a cluster leak and breaking the VM for that might be a
bit too much.

Still just printing a line on stderr and continuing otherwise doesn't
feel quite right, the user will usually miss the message because it ends
up in the log of a seemingly well working guest and if printed
unconditionally, it may still flood the logs.

Eric, would management be able to make something useful out of this if
we sent a QMP event?

Kevin