Re: [RFC PATCH v2 14/26] qcow2: Add subcluster support to qcow2_get_cluster_offset()

[Max Reitz]

On 08.11.19 16:42, Alberto Garcia wrote:
> On Mon 04 Nov 2019 03:58:57 PM CET, Max Reitz wrote:

[...]

>>> @@ -514,8 +499,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, 
>>> uint64_t offset,
>>
>> I suppose this is get_subcluster_offset now.
> 
> Hmmm no, this returns the actual host cluster offset, then the caller
> uses offset_into_cluster() to get the final value (which doesn't need to
> be subcluster-aligned anyway).

It still returns the subcluster type, though.  Which makes the whole
thing kind of a weird chimera...  Maybe it would work better if the
subcluster type would be stored in a pointer parameter, so it’d be clear?

But does it have actually have any advantages to on one hand return a
cluster-aligned host offset and on the other return a bytes count that
starts at the mid-cluster offset?  That’s weird already.  Maybe the
offset that’s returned should just not be cluster-aligned and the whole
function be called qcow2_get_host_offset().

I suppose one reason (maybe the only one?) to return aligned offsets is
for compressed clusters.  It would be another kind of magic to return
aligned offsets only for them.  But maybe it’s worth it?  (Judging from
a quick glance, it doesn’t look too difficult to me to modify the
callers to accept a mid-cluster host offset.)

(Another thing I just noticed is that the comment above
qcow2_get_cluster_offset() needs some fixing, because it still refers to
whole clusters.)

Max

signature.asc
Description: OpenPGP digital signature