On 01.07.20 18:26, Alberto Garcia wrote:
> On Wed 01 Jul 2020 02:52:14 PM CEST, Max Reitz wrote:
>>> if (l2_entry & QCOW_OFLAG_COMPRESSED) {
>>> return QCOW2_CLUSTER_COMPRESSED;
>>> - } else if (l2_entry & QCOW_OFLAG_ZERO) {
>>> + } else if ((l2_entry & QCOW_OFLAG_ZERO) && !has_subclusters(s)) {
>>
>> OK, so now qcow2_get_cluster_type() reports zero clusters to be normal
>> or unallocated clusters when there are subclusters. Seems weird to
>> me, because zero clusters are invalid clusters then.
>
> I'm actually hesitant about this.
>
> In extended L2 entries QCOW_OFLAG_ZERO does not have any meaning so
> technically it doesn't need to be checked any more than the other
> reserved bits (1 to 8).Good point. That convinces me. > The reason why we would want to check it is, of course, because that bit > does have a meaning in regular L2 entries. > > But that bit is ignored in images with subclusters so the only reason > why we would check it is to report corruption, not because we need to > know its value. Sure. But isn’t that the whole point of having QCOW2_SUBCLUSTER_INVALID in the first place? > It's true that we do check it in v2 images, although in that case the > entries are otherwise identical and there is a way to convert between > both types. > >> I preferred just reporting them as zero clusters and letting the >> caller deal with it, because it does mean an error in the image and so >> it should be reported. > > Another alternative would be to add QCOW2_CLUSTER_INVALID and we could > even include there other cases like unaligned offsets and things like > that. But that would also affect the code that repairs corrupted images. Interesting. Well, and that’d be definitely too much for this series, as you already said. So: Reviewed-by: Max Reitz <mre...@redhat.com>
signature.asc
Description: OpenPGP digital signature
