Re: [RFC PATCH v2 12/26] qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER

From: Max Reitz <mreitz@redhat.com>
To: Alberto Garcia <berto@igalia.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	Anton Nefedov <anton.nefedov@virtuozzo.com>,
	qemu-block@nongnu.org,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	"Denis V . Lunev" <den@openvz.org>
Subject: Re: [RFC PATCH v2 12/26] qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER
Date: Mon, 4 Nov 2019 14:10:37 +0100	[thread overview]
Message-ID: <016246de-df5c-b2a3-2d2d-b99ff7277fc5@redhat.com> (raw)
In-Reply-To: <w51tv7jssjf.fsf@maestria.local.igalia.com>

[-- Attachment #1.1: Type: text/plain, Size: 1813 bytes --]

On 04.11.19 14:03, Alberto Garcia wrote:
> On Mon 04 Nov 2019 01:57:42 PM CET, Max Reitz wrote:
>> On 26.10.19 23:25, Alberto Garcia wrote:
>>> In the previous patch we added a new QCow2ClusterType named
>>> QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER. There is a couple of places
>>> where this new value needs to be handled, and that is what this patch
>>> does.
>>>
>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>>  block/qcow2.c | 13 +++++++++----
>>>  1 file changed, 9 insertions(+), 4 deletions(-)
>> This patch deals with everything in qcow2.c.  There are more places that
>> reference QCOW2_CLUSTER_* constants elsewhere, and I suppose most of
>> them are handled by the following patches.
>>
>> But I wonder what the criterion is on where it needs to be handled and
>> where it’s OK not to.  Right now it looks to me like it’s a bit
>> arbitrary maybe?  But I suppose I’ll just have to wait until after the
>> next patches.
> 
> This is the part of the series that I'm the least happy about, because
> the existing qcow2_get_cluster_type() can never return this new value, I
> only updated the cases where this can actually happen.
> 
> I'm still considering a different approach for this.
I still don’t know what you’re doing in the later patches, but to me it
looks a bit like you don’t dare breaking up the existing structure that
just deals with clusters.

If that is so, I think it will help to make a clear cut between what
concerns subclusters and what concerns clusters at a whole.
QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER shouldn’t be in QCow2ClusterType;
there should be a separate QCow2SubclusterType.

OTOH, that would require more modifications, but (naïvely) I believe
that would make for the cleaner interface in the end.

Max

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]