Re: [PATCH v10 08/10] qcow2.py: Introduce '-j' key to dump in JSON format

[Andrey Shinkevich]

On 16.07.2020 13:20, Vladimir Sementsov-Ogievskiy wrote:
> 14.07.2020 00:36, Andrey Shinkevich wrote:
> > Add the command key to the qcow2.py arguments list to dump QCOW2
> > metadata in JSON format. Here is the suggested way to do that. The
> > implementation of the dump in JSON format is in the patch that follows.
> >
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > ---
> >   tests/qemu-iotests/qcow2.py        | 19 +++++++++++++++----
> >   tests/qemu-iotests/qcow2_format.py | 16 ++++++++--------
> >   2 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
> > index 0910e6a..7402279 100755
...
> > @@ -134,6 +137,11 @@ cmds = [
> >       def main(filename, cmd, args):
> > +    global dump_json
> > +    dump_json = '-j' in sys.argv
> > +    if dump_json:
> > +        sys.argv.remove('-j')
> > +        args.remove('-j')
>
> I'd prefer to access sys.argv in one place (in "if __name__ ...").

In case we move the block

    global dump_json
    dump_json = '-j' in sys.argv

        if dump_json:
            sys.argv.remove('-j')

from main() to the body of "if __name__ == '__main__'"

we get the following error -

"SyntaxError: name 'dump_json' is assigned to before global declaration"


Andrey


> >       fd = open(filename, "r+b")
> >       try:
> >           for name, handler, num_args, desc in cmds:
> > @@ -151,11 +159,14 @@ def main(filename, cmd, args):
> >       def usage():
> > -    print("Usage: %s <file> <cmd> [<arg>, ...]" % sys.argv[0])
> > +    print("Usage: %s <file> <cmd> [<arg>, ...] [<key>, ...]" % 
> > sys.argv[0])
> >       print("")
> >       print("Supported commands:")
> >       for name, handler, num_args, desc in cmds:
> >           print("    %-20s - %s" % (name, desc))
> > +    print("")
> > +    print("Supported keys:")
> > +    print("    %-20s - %s" % ('-j', 'Dump in JSON format'))
> >       if __name__ == '__main__':
> > diff --git a/tests/qemu-iotests/qcow2_format.py 
> > b/tests/qemu-iotests/qcow2_format.py
> > index 2c78d46..e0e14b5 100644
> > --- a/tests/qemu-iotests/qcow2_format.py
> > +++ b/tests/qemu-iotests/qcow2_format.py
> > @@ -109,7 +109,7 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
> >           self.__dict__ = dict((field[2], values[i])
> >                                for i, field in enumerate(self.fields))
> >   -    def dump(self):
> > +    def dump(self, dump_json=None):
> >           for f in self.fields:
> >               value = self.__dict__[f[2]]
> >               if isinstance(f[1], str):
> > @@ -145,8 +145,8 @@ class Qcow2BitmapExt(Qcow2Struct):
> >               [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
> >                for _ in range(self.nb_bitmaps)]
> >   -    def dump(self):
> > -        super().dump()
> > +    def dump(self, dump_json=None):
>
> strange to make None the default value for boolean. Why not just False?
>
> Also, why not call it just "json"? We are already in "dump" context, 
> no needs to add prefix to the name.
>
> > +        super().dump(dump_json)
> >           for entry in self.bitmap_directory:
> >               print()
> >               entry.dump()
>
> How will it work? You are interleaving json dump and non-json?
>
> Looking at this, I think that json and non-json dumps has more 
> differences than similarities, and it probably simpler to make a 
> separate function dump_json.. But I'm absolutely against an option, if 
> it will be done consistently.
>
> > @@ -190,7 +190,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
> >           self.bitmap_table = Qcow2BitmapTable(raw_table=table,
> > cluster_size=self.cluster_size)
> >   -    def dump(self):
> > +    def dump(self, dump_json=None):
> >           print(f'{"Bitmap name":<25} {self.name}')
> >           super(Qcow2BitmapDirEntry, self).dump()
> >           self.bitmap_table.dump()
>
> Here the new option just not passed to the nested dump() calls..
>
> > @@ -291,13 +291,13 @@ class QcowHeaderExtension(Qcow2Struct):
> >                       data_str = '<binary>'
> >                   self.data_str = data_str
> >   -    def dump(self):
> > +    def dump(self, dump_json=None):
> >           super().dump()
> >             if self.obj is None:
> >               print(f'{"data":<25} {self.data_str}')
> >           else:
> > -            self.obj.dump()
> > +            self.obj.dump(dump_json)
> >         @classmethod
> >       def create(cls, magic, data):
> > @@ -396,8 +396,8 @@ class QcowHeader(Qcow2Struct):
> >           buf = buf[0:header_bytes-1]
> >           fd.write(buf)
> >   -    def dump_extensions(self):
> > +    def dump_extensions(self, dump_json=None):
> >           for ex in self.extensions:
> >               print('Header extension:')
> > -            ex.dump()
> > +            ex.dump(dump_json)
> >               print()