Re: [PATCH v10 09/10] qcow2_format.py: collect fields to dump in JSON format - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: kwolf@redhat.com, den@openvz.org, qemu-devel@nongnu.org,
	mreitz@redhat.com
Subject: Re: [PATCH v10 09/10] qcow2_format.py: collect fields to dump in JSON format
Date: Thu, 16 Jul 2020 19:08:31 +0300	[thread overview]
Message-ID: <50904a8c-8116-fb28-6115-2f1762677820@virtuozzo.com> (raw)
In-Reply-To: <225afe71-36a5-803a-97f9-4f2eb3f355b9@virtuozzo.com>

16.07.2020 18:52, Andrey Shinkevich wrote:
> On 16.07.2020 18:40, Vladimir Sementsov-Ogievskiy wrote:
>> 16.07.2020 18:34, Andrey Shinkevich wrote:
>>> On 16.07.2020 13:24, Vladimir Sementsov-Ogievskiy wrote:
>>>> 14.07.2020 00:36, Andrey Shinkevich wrote:
>>>>> As __dict__ is being extended with class members we do not want to
>>>>> print, make a light copy of the initial __dict__ and extend the copy
>>>>> by adding lists we have to print in the JSON output.
>>>>>
>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>> ---
>>>>>   tests/qemu-iotests/qcow2_format.py | 4 ++++
>>>>>   1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
>>>>> index e0e14b5..83c3482 100644
>>>>> --- a/tests/qemu-iotests/qcow2_format.py
>>>>> +++ b/tests/qemu-iotests/qcow2_format.py
>>>>> @@ -109,6 +109,8 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
>>>>>           self.__dict__ = dict((field[2], values[i])
>>>>>                                for i, field in enumerate(self.fields))
>>>>>   +        self.fields_dict = self.__dict__.copy()
>>>>
>>>> No, I don't like that. Keeping two copies of all the data is bad idea. If you want to select some fields, add a method (dump_json() ?) Which will collect the fields you want in a dict and return that new dict. But don't store object attributes twice.
> 
> 
> That is what I did in the versions before but it looks clumsy to me. Every single class lists almost all the items of the __dict__ again in the additional method.
> 

Most of them should be listed automatically by base class method, which will iterate through .fields

> 
>>>>
>>>
>>> Not really. It makes a light copy that stores only references to the desired fields.
>>>
>>
>> I'm not about memory consumption, I'm about the design. Keeping two representations of same thing is always painful to maintain.
>>
>>>
>>>>> +
>>>>>       def dump(self, dump_json=None):
>>>>>           for f in self.fields:
>>>>>               value = self.__dict__[f[2]]
>>>>> @@ -144,6 +146,7 @@ class Qcow2BitmapExt(Qcow2Struct):
>>>>>           self.bitmap_directory = \
>>>>>               [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
>>>>>                for _ in range(self.nb_bitmaps)]
>>>>> + self.fields_dict.update(bitmap_directory=self.bitmap_directory)
>>>>>         def dump(self, dump_json=None):
>>>>>           super().dump(dump_json)
>>>>> @@ -189,6 +192,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
>>>>>           table = [e[0] for e in struct.iter_unpack('>Q', fd.read(table_size))]
>>>>>           self.bitmap_table = Qcow2BitmapTable(raw_table=table,
>>>>> cluster_size=self.cluster_size)
>>>>> + self.fields_dict.update(bitmap_table=self.bitmap_table)
>>>>>         def dump(self, dump_json=None):
>>>>>           print(f'{"Bitmap name":<25} {self.name}')
>>>>>
>>>>
>>>>
>>
>>


-- 
Best regards,
Vladimir