Re: [PATCH v8 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method

2020-07-13 Thread Vladimir Sementsov-Ogievskiy 

13.07.2020 07:49, Andrey Shinkevich wrote:

On 11.07.2020 19:34, Vladimir Sementsov-Ogievskiy wrote:

03.07.2020 16:13, Andrey Shinkevich wrote:

There are two ways to initialize a class derived from Qcow2Struct:
1. Pass a block of binary data to the constructor.
2. Pass the file descriptor to allow reading the file from constructor.
Let's change the Qcow2BitmapExt initialization method from 1 to 2 to
support a scattered reading in the initialization chain.
The implementation comes with the patch that follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/qcow2_format.py | 14 --
  1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2f3681b..1435e34 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -63,7 +63,8 @@ class Qcow2StructMeta(type):
    class Qcow2Struct(metaclass=Qcow2StructMeta):
  -    """Qcow2Struct: base class for qcow2 data structures
+    """
+    Qcow2Struct: base class for qcow2 data structures


Unrelated chunk. And why?



To conform to the common style for comments in the file as it is at

class QcowHeaderExtension::__init__()


It's OK to additionally fix/change style in the chunk where you change some 
logic anyway. But changing style in another places - I think it doesn't worth 
it. Remember that it may make patch porting more complicated for nothing.

And this is not only my position, https://wiki.qemu.org/Contribute/SubmitAPatch directly 
statates it in "Don't include irrelevant changes" paragraph:

In particular, don't include formatting, coding style or whitespace changes 
to bits of code that would otherwise not be touched by the patch.







    Successors should define fields class variable, which is: list of 
tuples,
  each of three elements:
@@ -113,6 +114,9 @@ class Qcow2BitmapExt(Qcow2Struct):
  ('u64', '{:#x}', 'bitmap_directory_offset')
  )
  +    def __init__(self, fd):
+    super().__init__(fd=fd)


this does nothing. We inherit the __init__ of super class, no need to define it 
just to call same __init__.


+
    QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
  @@ -173,7 +177,13 @@ class QcowHeaderExtension(Qcow2Struct):
  self.data_str = data_str
    if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
-    self.obj = Qcow2BitmapExt(data=self.data)
+    assert fd is not None
+    position = fd.tell()
+    # Step back to reread data


This definitely shows that we are doing something wrong



For Qcow2BitmapExt, we need both fd and data and they are mutualy exclusive

in the constructor of the class Qcow2Struct. Rereading the bitmap extension

is a solution without changing the Qcow2Struct. Any other suggestion?



I think, we want to modify QcowHeaderExtension so that it creates/reads its 
children appropriately. Probably, for bitmaps extension exclusively we pass fd 
to Qcow2BitmapExt, not reading data by ourselves, and for other extensions keep 
current semantics of just reading data by hand (or you may add a simple class 
Qcow2Ext.







+    padded = (self.length + 7) & ~7
+    fd.seek(-padded, 1)
+    self.obj = Qcow2BitmapExt(fd=fd)
+    fd.seek(position)
  else:
  self.obj = None







--
Best regards,
Vladimir

Re: [PATCH v8 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method

2020-07-12 Thread Andrey Shinkevich 

On 11.07.2020 19:34, Vladimir Sementsov-Ogievskiy wrote:

03.07.2020 16:13, Andrey Shinkevich wrote:

There are two ways to initialize a class derived from Qcow2Struct:
1. Pass a block of binary data to the constructor.
2. Pass the file descriptor to allow reading the file from constructor.
Let's change the Qcow2BitmapExt initialization method from 1 to 2 to
support a scattered reading in the initialization chain.
The implementation comes with the patch that follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/qcow2_format.py | 14 --
  1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py

index 2f3681b..1435e34 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -63,7 +63,8 @@ class Qcow2StructMeta(type):
    class Qcow2Struct(metaclass=Qcow2StructMeta):
  -    """Qcow2Struct: base class for qcow2 data structures
+    """
+    Qcow2Struct: base class for qcow2 data structures


Unrelated chunk. And why?



To conform to the common style for comments in the file as it is at

class QcowHeaderExtension::__init__()




    Successors should define fields class variable, which is: 
list of tuples,

  each of three elements:
@@ -113,6 +114,9 @@ class Qcow2BitmapExt(Qcow2Struct):
  ('u64', '{:#x}', 'bitmap_directory_offset')
  )
  +    def __init__(self, fd):
+    super().__init__(fd=fd)


this does nothing. We inherit the __init__ of super class, no need to 
define it just to call same __init__.



+
    QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
  @@ -173,7 +177,13 @@ class QcowHeaderExtension(Qcow2Struct):
  self.data_str = data_str
    if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
-    self.obj = Qcow2BitmapExt(data=self.data)
+    assert fd is not None
+    position = fd.tell()
+    # Step back to reread data


This definitely shows that we are doing something wrong



For Qcow2BitmapExt, we need both fd and data and they are mutualy exclusive

in the constructor of the class Qcow2Struct. Rereading the bitmap extension

is a solution without changing the Qcow2Struct. Any other suggestion?

Andrey





+    padded = (self.length + 7) & ~7
+    fd.seek(-padded, 1)
+    self.obj = Qcow2BitmapExt(fd=fd)
+    fd.seek(position)
  else:
  self.obj = None

Re: [PATCH v8 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method

2020-07-11 Thread Vladimir Sementsov-Ogievskiy 

03.07.2020 16:13, Andrey Shinkevich wrote:

There are two ways to initialize a class derived from Qcow2Struct:
1. Pass a block of binary data to the constructor.
2. Pass the file descriptor to allow reading the file from constructor.
Let's change the Qcow2BitmapExt initialization method from 1 to 2 to
support a scattered reading in the initialization chain.
The implementation comes with the patch that follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/qcow2_format.py | 14 --
  1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2f3681b..1435e34 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -63,7 +63,8 @@ class Qcow2StructMeta(type):
  
  class Qcow2Struct(metaclass=Qcow2StructMeta):
  
-"""Qcow2Struct: base class for qcow2 data structures

+"""
+Qcow2Struct: base class for qcow2 data structures


Unrelated chunk. And why?

  
  Successors should define fields class variable, which is: list of tuples,

  each of three elements:
@@ -113,6 +114,9 @@ class Qcow2BitmapExt(Qcow2Struct):
  ('u64', '{:#x}', 'bitmap_directory_offset')
  )
  
+def __init__(self, fd):

+super().__init__(fd=fd)


this does nothing. We inherit the __init__ of super class, no need to define it 
just to call same __init__.


+
  
  QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
  
@@ -173,7 +177,13 @@ class QcowHeaderExtension(Qcow2Struct):

  self.data_str = data_str
  
  if self.magic == QCOW2_EXT_MAGIC_BITMAPS:

-self.obj = Qcow2BitmapExt(data=self.data)
+assert fd is not None
+position = fd.tell()
+# Step back to reread data


This definitely shows that we are doing something wrong


+padded = (self.length + 7) & ~7
+fd.seek(-padded, 1)
+self.obj = Qcow2BitmapExt(fd=fd)
+fd.seek(position)
  else:
  self.obj = None
  




--
Best regards,
Vladimir

[PATCH v8 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method

2020-07-03 Thread Andrey Shinkevich 
There are two ways to initialize a class derived from Qcow2Struct:
1. Pass a block of binary data to the constructor.
2. Pass the file descriptor to allow reading the file from constructor.
Let's change the Qcow2BitmapExt initialization method from 1 to 2 to
support a scattered reading in the initialization chain.
The implementation comes with the patch that follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
 tests/qemu-iotests/qcow2_format.py | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2f3681b..1435e34 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -63,7 +63,8 @@ class Qcow2StructMeta(type):
 
 class Qcow2Struct(metaclass=Qcow2StructMeta):
 
-"""Qcow2Struct: base class for qcow2 data structures
+"""
+Qcow2Struct: base class for qcow2 data structures
 
 Successors should define fields class variable, which is: list of tuples,
 each of three elements:
@@ -113,6 +114,9 @@ class Qcow2BitmapExt(Qcow2Struct):
 ('u64', '{:#x}', 'bitmap_directory_offset')
 )
 
+def __init__(self, fd):
+super().__init__(fd=fd)
+
 
 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
 
@@ -173,7 +177,13 @@ class QcowHeaderExtension(Qcow2Struct):
 self.data_str = data_str
 
 if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
-self.obj = Qcow2BitmapExt(data=self.data)
+assert fd is not None
+position = fd.tell()
+# Step back to reread data
+padded = (self.length + 7) & ~7
+fd.seek(-padded, 1)
+self.obj = Qcow2BitmapExt(fd=fd)
+fd.seek(position)
 else:
 self.obj = None
 
-- 
1.8.3.1