Re: [PATCH v8 05/10] qcow2_format.py: Dump bitmap directory information

[Vladimir Sementsov-Ogievskiy]

13.07.2020 10:07, Andrey Shinkevich wrote:
> On 11.07.2020 22:11, Vladimir Sementsov-Ogievskiy wrote:
> > 03.07.2020 16:13, Andrey Shinkevich wrote:
> > > Read and dump entries from the bitmap directory of QCOW2 image.
> > > It extends the output in the test case #291.
> ...
> > >   diff --git a/tests/qemu-iotests/qcow2_format.py 
> > > b/tests/qemu-iotests/qcow2_format.py
> > > index d8c058d..7c0dc9a 100644
> > > --- a/tests/qemu-iotests/qcow2_format.py
> > > +++ b/tests/qemu-iotests/qcow2_format.py
> > > @@ -132,6 +132,50 @@ class Qcow2BitmapExt(Qcow2Struct):
> > >         def __init__(self, fd):
> > >           super().__init__(fd=fd)
> > > +        self.read_bitmap_directory(fd)
> > > +
> > > +    def read_bitmap_directory(self, fd):
> > > +        fd.seek(self.bitmap_directory_offset)
> > > +        self.bitmap_directory = \
> > > +            [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]
> >
> > sounds good. I think, we should restore fd position after reading 
> > bitmap_directory, to point at the end of extension, to not break further 
> > extensions loading
> Yes, it is done in the constructor of QcowHeaderExtension:
>
> if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
>
> ...
>
> position = fd.tell()
>
> ...
>
> self.obj = Qcow2BitmapExt(fd=fd)
>
> fd.seek(position)

I don't like it. If you want caller to care about fd, caller should know size 
of created child. But passing fd to constructor implies that caller not aware 
of size of new created structure. So I think good api is: constuctor starts to 
read the structure and left after this structure on exit from consturctor (so, 
caller may read following structures). Constructor may read some nested 
structures, but is responsible for restoring fd after it.


--
Best regards,
Vladimir