| Message ID | 1550834773-873512-1-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iotests: handle TypeError for Python3 in test 242 | expand |
On 2/22/19 5:26 AM, Andrey Shinkevich wrote: > The data type for bytes in Python3 differs from the one in Python2. > Those cases should be managed separately. > > v1: > In the first version, the TypeError in Python3 was handled as the > exception. > Discussed in the e-mail thread with the Message ID: > <1550519997-253534-1-git-send-email-andrey.shinkevich@virtuozzo.com> This paragraph belongs... > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > Reported-by: Kevin Wolf <kwolf@redhat.com> > --- ...here, after the --- marker. It is useful to reviewers, but does not need to land in the long-term git history (a year from now, we won't care if it was v2 or v10 that landed, nor how it changed from the versions that didn't land). I can clean it up on staging. Reviewed-by: Eric Blake <eblake@redhat.com> Will stage through my NBD tree.
On 2/22/19 6:26 AM, Andrey Shinkevich wrote: > The data type for bytes in Python3 differs from the one in Python2. > Those cases should be managed separately. > > v1: > In the first version, the TypeError in Python3 was handled as the > exception. > Discussed in the e-mail thread with the Message ID: > <1550519997-253534-1-git-send-email-andrey.shinkevich@virtuozzo.com> > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > Reported-by: Kevin Wolf <kwolf@redhat.com> > --- > tests/qemu-iotests/242 | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242 > index 16c65ed..446fbf8 100755 > --- a/tests/qemu-iotests/242 > +++ b/tests/qemu-iotests/242 > @@ -20,6 +20,7 @@ > > import iotests > import json > +import sys > from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \ > file_path, img_info_log, log, filter_qemu_io > > @@ -65,9 +66,12 @@ def toggle_flag(offset): > with open(disk, "r+b") as f: > f.seek(offset, 0) > c = f.read(1) > - toggled = chr(ord(c) ^ bitmap_flag_unknown) > + toggled = ord(c) ^ bitmap_flag_unknown > f.seek(-1, 1) > - f.write(toggled) > + if sys.version_info.major >= 3: > + f.write(bytes([toggled])) > + else: > + f.write(chr(toggled)) > I originally suggested: sys.version_info.major == 2: ... Because this is already present on other tests, and IIRC Max mentioned using this as an easy to find flag the moment Python 2 support is to be dropped. But, looking for "sys.version_info.major" is just as effective, so: Reviewed-by: Cleber Rosa <crosa@redhat.com> > > qemu_img_create('-f', iotests.imgfmt, disk, '1M') >
22.02.2019 14:26, Andrey Shinkevich wrote: > The data type for bytes in Python3 differs from the one in Python2. > Those cases should be managed separately. > > v1: > In the first version, the TypeError in Python3 was handled as the > exception. > Discussed in the e-mail thread with the Message ID: > <1550519997-253534-1-git-send-email-andrey.shinkevich@virtuozzo.com> > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > Reported-by: Kevin Wolf <kwolf@redhat.com> > --- > tests/qemu-iotests/242 | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242 > index 16c65ed..446fbf8 100755 > --- a/tests/qemu-iotests/242 > +++ b/tests/qemu-iotests/242 > @@ -20,6 +20,7 @@ > > import iotests > import json > +import sys > from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \ > file_path, img_info_log, log, filter_qemu_io > > @@ -65,9 +66,12 @@ def toggle_flag(offset): > with open(disk, "r+b") as f: > f.seek(offset, 0) > c = f.read(1) > - toggled = chr(ord(c) ^ bitmap_flag_unknown) > + toggled = ord(c) ^ bitmap_flag_unknown > f.seek(-1, 1) > - f.write(toggled) > + if sys.version_info.major >= 3: > + f.write(bytes([toggled])) > + else: > + f.write(chr(toggled)) > > > qemu_img_create('-f', iotests.imgfmt, disk, '1M') > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 22/02/2019 18:20, Cleber Rosa wrote: > > > On 2/22/19 6:26 AM, Andrey Shinkevich wrote: >> The data type for bytes in Python3 differs from the one in Python2. >> Those cases should be managed separately. >> >> v1: >> In the first version, the TypeError in Python3 was handled as the >> exception. >> Discussed in the e-mail thread with the Message ID: >> <1550519997-253534-1-git-send-email-andrey.shinkevich@virtuozzo.com> >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> Reported-by: Kevin Wolf <kwolf@redhat.com> >> --- >> tests/qemu-iotests/242 | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242 >> index 16c65ed..446fbf8 100755 >> --- a/tests/qemu-iotests/242 >> +++ b/tests/qemu-iotests/242 >> @@ -20,6 +20,7 @@ >> >> import iotests >> import json >> +import sys >> from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \ >> file_path, img_info_log, log, filter_qemu_io >> >> @@ -65,9 +66,12 @@ def toggle_flag(offset): >> with open(disk, "r+b") as f: >> f.seek(offset, 0) >> c = f.read(1) >> - toggled = chr(ord(c) ^ bitmap_flag_unknown) >> + toggled = ord(c) ^ bitmap_flag_unknown >> f.seek(-1, 1) >> - f.write(toggled) >> + if sys.version_info.major >= 3: >> + f.write(bytes([toggled])) >> + else: >> + f.write(chr(toggled)) >> > > I originally suggested: > > sys.version_info.major == 2: > ... > > Because this is already present on other tests, and IIRC Max mentioned > using this as an easy to find flag the moment Python 2 support is to be > dropped. But, looking for "sys.version_info.major" is just as > effective, so: > > Reviewed-by: Cleber Rosa <crosa@redhat.com> > Thank you very much, Cleber. Your review is helpful. >> >> qemu_img_create('-f', iotests.imgfmt, disk, '1M') >>
On 22/02/2019 17:53, Eric Blake wrote: > On 2/22/19 5:26 AM, Andrey Shinkevich wrote: >> The data type for bytes in Python3 differs from the one in Python2. >> Those cases should be managed separately. >> >> v1: >> In the first version, the TypeError in Python3 was handled as the >> exception. >> Discussed in the e-mail thread with the Message ID: >> <1550519997-253534-1-git-send-email-andrey.shinkevich@virtuozzo.com> > > This paragraph belongs... > >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> Reported-by: Kevin Wolf <kwolf@redhat.com> >> --- > > ...here, after the --- marker. It is useful to reviewers, but does not > need to land in the long-term git history (a year from now, we won't > care if it was v2 or v10 that landed, nor how it changed from the > versions that didn't land). I can clean it up on staging. > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Will stage through my NBD tree. > Yes, please, Eric. Actually, I had missed to put that paragraph bellow the dashes while composing it as a cover letter. That was my fault. Acknowledged with thanks. I will take your comment into account for future.
On Fri, Feb 22, 2019 at 02:26:13PM +0300, Andrey Shinkevich wrote: > The data type for bytes in Python3 differs from the one in Python2. > Those cases should be managed separately. > > v1: > In the first version, the TypeError in Python3 was handled as the > exception. > Discussed in the e-mail thread with the Message ID: > <1550519997-253534-1-git-send-email-andrey.shinkevich@virtuozzo.com> > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > Reported-by: Kevin Wolf <kwolf@redhat.com> > --- > tests/qemu-iotests/242 | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242 > index 16c65ed..446fbf8 100755 > --- a/tests/qemu-iotests/242 > +++ b/tests/qemu-iotests/242 > @@ -20,6 +20,7 @@ > > import iotests > import json > +import sys > from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \ > file_path, img_info_log, log, filter_qemu_io > > @@ -65,9 +66,12 @@ def toggle_flag(offset): > with open(disk, "r+b") as f: > f.seek(offset, 0) > c = f.read(1) > - toggled = chr(ord(c) ^ bitmap_flag_unknown) > + toggled = ord(c) ^ bitmap_flag_unknown > f.seek(-1, 1) > - f.write(toggled) > + if sys.version_info.major >= 3: > + f.write(bytes([toggled])) > + else: > + f.write(chr(toggled)) Pretending we are dealing with text strings and using str/ord is a python2-specific quirk. I think it would be nice to get rid of it. Python 2 has bytearray(), which behaves more similarly to the bytes type from Python 3. If we use it, we can make the code more python3-like: diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242 index 16c65edcd7..7794fd4a70 100755 --- a/tests/qemu-iotests/242 +++ b/tests/qemu-iotests/242 @@ -64,10 +64,12 @@ def write_to_disk(offset, size): def toggle_flag(offset): with open(disk, "r+b") as f: f.seek(offset, 0) - c = f.read(1) - toggled = chr(ord(c) ^ bitmap_flag_unknown) + # The casts to bytearray() below are only necessary + # for Python 2 compatibility + c = bytearray(f.read(1))[0] + toggled = c ^ bitmap_flag_unknown f.seek(-1, 1) - f.write(toggled) + f.write(bytearray([toggled])) qemu_img_create('-f', iotests.imgfmt, disk, '1M')
On Mon, Feb 25, 2019 at 10:36 PM Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Feb 22, 2019 at 02:26:13PM +0300, Andrey Shinkevich wrote: > > The data type for bytes in Python3 differs from the one in Python2. > > Those cases should be managed separately. > > > > v1: > > In the first version, the TypeError in Python3 was handled as the > > exception. > > Discussed in the e-mail thread with the Message ID: > > <1550519997-253534-1-git-send-email-andrey.shinkevich@virtuozzo.com> > > > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > > Reported-by: Kevin Wolf <kwolf@redhat.com> > > --- > > tests/qemu-iotests/242 | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242 > > index 16c65ed..446fbf8 100755 > > --- a/tests/qemu-iotests/242 > > +++ b/tests/qemu-iotests/242 > > @@ -20,6 +20,7 @@ > > > > import iotests > > import json > > +import sys > > from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \ > > file_path, img_info_log, log, filter_qemu_io > > > > @@ -65,9 +66,12 @@ def toggle_flag(offset): > > with open(disk, "r+b") as f: > > f.seek(offset, 0) > > c = f.read(1) > > - toggled = chr(ord(c) ^ bitmap_flag_unknown) > > + toggled = ord(c) ^ bitmap_flag_unknown > > f.seek(-1, 1) > > - f.write(toggled) > > + if sys.version_info.major >= 3: > > + f.write(bytes([toggled])) > > + else: > > + f.write(chr(toggled)) > > Pretending we are dealing with text strings and using str/ord is > a python2-specific quirk. I think it would be nice to get rid of > it. > > Python 2 has bytearray(), which behaves more similarly to the > bytes type from Python 3. If we use it, we can make the code > more python3-like: > > diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242 > index 16c65edcd7..7794fd4a70 100755 > --- a/tests/qemu-iotests/242 > +++ b/tests/qemu-iotests/242 > @@ -64,10 +64,12 @@ def write_to_disk(offset, size): > def toggle_flag(offset): > with open(disk, "r+b") as f: > f.seek(offset, 0) > - c = f.read(1) > - toggled = chr(ord(c) ^ bitmap_flag_unknown) > + # The casts to bytearray() below are only necessary > + # for Python 2 compatibility > + c = bytearray(f.read(1))[0] > This is simpler and makes the intent of the code more clear: flag, = struct.unpack("B", f.read(1)) > + toggled = c ^ bitmap_flag_unknown > f.seek(-1, 1) > - f.write(toggled) > + f.write(bytearray([toggled])) > For consistency, we can use struct.pack here: f.write(struct.pack("B", toggled)) Nir > > > qemu_img_create('-f', iotests.imgfmt, disk, '1M') > > -- > Eduardo > >
On 26/02/2019 04:42, Nir Soffer wrote: > On Mon, Feb 25, 2019 at 10:36 PM Eduardo Habkost <ehabkost@redhat.com > <mailto:ehabkost@redhat.com>> wrote: > > On Fri, Feb 22, 2019 at 02:26:13PM +0300, Andrey Shinkevich wrote: > > The data type for bytes in Python3 differs from the one in Python2. > > Those cases should be managed separately. > > > > v1: > > In the first version, the TypeError in Python3 was handled as the > > exception. > > Discussed in the e-mail thread with the Message ID: > > > <1550519997-253534-1-git-send-email-andrey.shinkevich@virtuozzo.com > <mailto:1550519997-253534-1-git-send-email-andrey.shinkevich@virtuozzo.com>> > > > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com > <mailto:andrey.shinkevich@virtuozzo.com>> > > Reported-by: Kevin Wolf <kwolf@redhat.com <mailto:kwolf@redhat.com>> > > --- > > tests/qemu-iotests/242 | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242 > > index 16c65ed..446fbf8 100755 > > --- a/tests/qemu-iotests/242 > > +++ b/tests/qemu-iotests/242 > > @@ -20,6 +20,7 @@ > > > > import iotests > > import json > > +import sys > > from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \ > > file_path, img_info_log, log, filter_qemu_io > > > > @@ -65,9 +66,12 @@ def toggle_flag(offset): > > with open(disk, "r+b") as f: > > f.seek(offset, 0) > > c = f.read(1) > > - toggled = chr(ord(c) ^ bitmap_flag_unknown) > > + toggled = ord(c) ^ bitmap_flag_unknown > > f.seek(-1, 1) > > - f.write(toggled) > > + if sys.version_info.major >= 3: > > + f.write(bytes([toggled])) > > + else: > > + f.write(chr(toggled)) > > Pretending we are dealing with text strings and using str/ord is > a python2-specific quirk. I think it would be nice to get rid of > it. > > Python 2 has bytearray(), which behaves more similarly to the > bytes type from Python 3. If we use it, we can make the code > more python3-like: > > diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242 > index 16c65edcd7..7794fd4a70 100755 > --- a/tests/qemu-iotests/242 > +++ b/tests/qemu-iotests/242 > @@ -64,10 +64,12 @@ def write_to_disk(offset, size): > def toggle_flag(offset): > with open(disk, "r+b") as f: > f.seek(offset, 0) > - c = f.read(1) > - toggled = chr(ord(c) ^ bitmap_flag_unknown) > + # The casts to bytearray() below are only necessary > + # for Python 2 compatibility > + c = bytearray(f.read(1))[0] > > > This is simpler and makes the intent of the code more clear: > > flag, = struct.unpack("B", f.read(1)) > > + toggled = c ^ bitmap_flag_unknown > f.seek(-1, 1) > - f.write(toggled) > + f.write(bytearray([toggled])) > > > For consistency, we can use struct.pack here: > > f.write(struct.pack("B", toggled)) > > Nir > Thank you all. I am OK with this approach. Will wait for Eric's response. > > > qemu_img_create('-f', iotests.imgfmt, disk, '1M') > > -- > Eduardo >
On Tue, Feb 26, 2019 at 03:42:11AM +0200, Nir Soffer wrote: > On Mon, Feb 25, 2019 at 10:36 PM Eduardo Habkost <ehabkost@redhat.com> [...] > > - c = f.read(1) > > - toggled = chr(ord(c) ^ bitmap_flag_unknown) > > + # The casts to bytearray() below are only necessary > > + # for Python 2 compatibility > > + c = bytearray(f.read(1))[0] > > > > This is simpler and makes the intent of the code more clear: > > flag, = struct.unpack("B", f.read(1)) > > > > + toggled = c ^ bitmap_flag_unknown > > f.seek(-1, 1) > > - f.write(toggled) > > + f.write(bytearray([toggled])) > > > > For consistency, we can use struct.pack here: > > f.write(struct.pack("B", toggled)) That's perfect. Thanks for the suggestion!
On 2/26/19 4:39 AM, Andrey Shinkevich wrote: >> +++ b/tests/qemu-iotests/242 >> @@ -64,10 +64,12 @@ def write_to_disk(offset, size): >> def toggle_flag(offset): >> with open(disk, "r+b") as f: >> f.seek(offset, 0) >> - c = f.read(1) >> - toggled = chr(ord(c) ^ bitmap_flag_unknown) >> + # The casts to bytearray() below are only necessary >> + # for Python 2 compatibility >> + c = bytearray(f.read(1))[0] >> >> >> This is simpler and makes the intent of the code more clear: >> >> flag, = struct.unpack("B", f.read(1)) >> >> + toggled = c ^ bitmap_flag_unknown >> f.seek(-1, 1) >> - f.write(toggled) >> + f.write(bytearray([toggled])) >> >> >> For consistency, we can use struct.pack here: >> >> f.write(struct.pack("B", toggled)) >> >> Nir >> > > Thank you all. I am OK with this approach. > Will wait for Eric's response. That looks better. Peter hasn't applied my pull request yet, so you have time to submit a formal v3 (making it easier for me to 'git am' it rather than reconstruct from this email), and then I will update my pull request to use this improved version.
On 26/02/2019 17:06, Eric Blake wrote: > On 2/26/19 4:39 AM, Andrey Shinkevich wrote: > >>> +++ b/tests/qemu-iotests/242 >>> @@ -64,10 +64,12 @@ def write_to_disk(offset, size): >>> def toggle_flag(offset): >>> with open(disk, "r+b") as f: >>> f.seek(offset, 0) >>> - c = f.read(1) >>> - toggled = chr(ord(c) ^ bitmap_flag_unknown) >>> + # The casts to bytearray() below are only necessary >>> + # for Python 2 compatibility >>> + c = bytearray(f.read(1))[0] >>> >>> >>> This is simpler and makes the intent of the code more clear: >>> >>> flag, = struct.unpack("B", f.read(1)) >>> >>> + toggled = c ^ bitmap_flag_unknown >>> f.seek(-1, 1) >>> - f.write(toggled) >>> + f.write(bytearray([toggled])) >>> >>> >>> For consistency, we can use struct.pack here: >>> >>> f.write(struct.pack("B", toggled)) >>> >>> Nir >>> >> >> Thank you all. I am OK with this approach. >> Will wait for Eric's response. > > That looks better. Peter hasn't applied my pull request yet, so you have > time to submit a formal v3 (making it easier for me to 'git am' it > rather than reconstruct from this email), and then I will update my pull > request to use this improved version. > Noted
diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242 index 16c65ed..446fbf8 100755 --- a/tests/qemu-iotests/242 +++ b/tests/qemu-iotests/242 @@ -20,6 +20,7 @@ import iotests import json +import sys from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \ file_path, img_info_log, log, filter_qemu_io @@ -65,9 +66,12 @@ def toggle_flag(offset): with open(disk, "r+b") as f: f.seek(offset, 0) c = f.read(1) - toggled = chr(ord(c) ^ bitmap_flag_unknown) + toggled = ord(c) ^ bitmap_flag_unknown f.seek(-1, 1) - f.write(toggled) + if sys.version_info.major >= 3: + f.write(bytes([toggled])) + else: + f.write(chr(toggled)) qemu_img_create('-f', iotests.imgfmt, disk, '1M')
The data type for bytes in Python3 differs from the one in Python2. Those cases should be managed separately. v1: In the first version, the TypeError in Python3 was handled as the exception. Discussed in the e-mail thread with the Message ID: <1550519997-253534-1-git-send-email-andrey.shinkevich@virtuozzo.com> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> Reported-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/242 | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)