Re: [PATCH v10 06/10] qcow2_format.py: pass cluster size to substructures

[Andrey Shinkevich]

On 16.07.2020 12:26, Vladimir Sementsov-Ogievskiy wrote:
> 14.07.2020 00:36, Andrey Shinkevich wrote:
> > The cluster size of an image is the QcowHeader class member and may be
> > obtained by dependent extension structures such as Qcow2BitmapExt for
> > further bitmap table details print.
> >
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > ---
> >   tests/qemu-iotests/qcow2_format.py | 18 +++++++++++-------
> >   1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/qcow2_format.py 
> > b/tests/qemu-iotests/qcow2_format.py
...
> >         def read_bitmap_directory(self, fd):
> >           fd.seek(self.bitmap_directory_offset)
> >           self.bitmap_directory = \
> > -            [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]
> > +            [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
> > +             for _ in range(self.nb_bitmaps)]
>
> Better to inline the bitmap directory loading code into __init__:
>
> Benefits:
>  1. Less code. read_bitmap_directory() is very small, used only in 
> __init__ and just not needed as a separate method. __init__ is very 
> small and simple too, so it's not a problem.
>  2. no need of extra self.cluster_size variable (you can use 
> cluster_size parameter directly)
>  3. keep all fd.seek logic in one method
>
> but it's not about this patch.

The idea behind the read_bitmap_directory() method was an encapsulation 
of reading the optional parameter. So, we can be flexible in future. 
Sure, there are prones and cons for that in the current implementation.


Andrey


> >         def dump(self):
> >           super().dump()
> > @@ -162,8 +164,9 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
...
> > @@ -244,7 +249,6 @@ class QcowHeaderExtension(Qcow2Struct):
> >                       data_str = '<binary>'
> >                   self.data_str = data_str
> >   -
>
> Unrelated style-fixing chunk, please don't do so.
>
> with it dropped:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> >       def dump(self):
> >           super().dump()
> >   @@ -316,7 +320,7 @@ class QcowHeader(Qcow2Struct):
> >               end = self.cluster_size
> >             while fd.tell() < end:
> > -            ext = QcowHeaderExtension(fd=fd)
> > +            ext = QcowHeaderExtension(fd=fd, 
> > cluster_size=self.cluster_size)
> >               if ext.magic == 0:
> >                   break
> >               else: