| Message ID | 20190208100922.19888-4-masashi.honma@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/4] tests: Decode VM output for python3 | expand |
On Fri, Feb 08, 2019 at 07:09:22PM +0900, Masashi Honma wrote: > The result of reading non blocked empty stream is different between python2 > and 3. The python2 sends "[Errno 11] Resource temporarily unavailable" > exception. The python3 could read "None" without exception. > So separate processing. That looks excessive pretty ugly. Wouldn't it be simpler to just add support for None return without separating processing based on version? I.e., something like this (and well, I'd merge in the applicable .decode() changes from patch 1/4 to this patch taking into account they alone do not really fix these same read() lines): diff --git a/tests/hwsim/vm/parallel-vm.py b/tests/hwsim/vm/parallel-vm.py index df6c19b..d2b0def 100755 --- a/tests/hwsim/vm/parallel-vm.py +++ b/tests/hwsim/vm/parallel-vm.py @@ -90,7 +90,10 @@ def vm_read_stdout(vm, i): ready = False try: - out = vm['proc'].stdout.read().decode() + out = vm['proc'].stdout.read() + if out == None: + return False + out = out.decode() except: return False logger.debug("VM[%d] stdout.read[%s]" % (i, out)) @@ -191,9 +194,11 @@ def show_progress(scr): running = True first_running = True try: - err = vm[i]['proc'].stderr.read().decode() - vm[i]['err'] += err - logger.debug("VM[%d] stderr.read[%s]" % (i, err)) + err = vm[i]['proc'].stderr.read() + if err != None: + err = err.decode() + vm[i]['err'] += err + logger.debug("VM[%d] stderr.read[%s]" % (i, err)) except: pass @@ -247,8 +252,10 @@ def show_progress(scr): running = True try: err = vm[i]['proc'].stderr.read() - vm[i]['err'] += err - logger.debug("VM[%d] stderr.read[%s]" % (i, err)) + if err != None: + err = err.decode() + vm[i]['err'] += err + logger.debug("VM[%d] stderr.read[%s]" % (i, err)) except: pass
On 2019/02/08 19:41, Jouni Malinen wrote: > That looks excessive pretty ugly. Wouldn't it be simpler to just add > support for None return without separating processing based on version? > > I.e., something like this (and well, I'd merge in the applicable > .decode() changes from patch 1/4 to this patch taking into account they > alone do not really fix these same read() lines): > > > diff --git a/tests/hwsim/vm/parallel-vm.py b/tests/hwsim/vm/parallel-vm.py > index df6c19b..d2b0def 100755 > --- a/tests/hwsim/vm/parallel-vm.py > +++ b/tests/hwsim/vm/parallel-vm.py > @@ -90,7 +90,10 @@ def vm_read_stdout(vm, i): > > ready = False > try: > - out = vm['proc'].stdout.read().decode() > + out = vm['proc'].stdout.read() > + if out == None: > + return False > + out = out.decode() > except: > return False > logger.debug("VM[%d] stdout.read[%s]" % (i, out)) > @@ -191,9 +194,11 @@ def show_progress(scr): > running = True > first_running = True > try: > - err = vm[i]['proc'].stderr.read().decode() > - vm[i]['err'] += err > - logger.debug("VM[%d] stderr.read[%s]" % (i, err)) > + err = vm[i]['proc'].stderr.read() > + if err != None: > + err = err.decode() > + vm[i]['err'] += err > + logger.debug("VM[%d] stderr.read[%s]" % (i, err)) > except: > pass > > @@ -247,8 +252,10 @@ def show_progress(scr): > running = True > try: > err = vm[i]['proc'].stderr.read() > - vm[i]['err'] += err > - logger.debug("VM[%d] stderr.read[%s]" % (i, err)) > + if err != None: > + err = err.decode() > + vm[i]['err'] += err > + logger.debug("VM[%d] stderr.read[%s]" % (i, err)) > except: > pass > > That looks excessive pretty ugly. Wouldn't it be simpler to just add > support for None return without separating processing based on version? I can understand because my previous modification is like it. My purpose of this patch is not to catch exception if not needed. Because catching exception makes debugging more difficult. Indeed, sometimes I needed to comment out try statements temporarily while doing python2 to 3 project. Which do you like ?
On Fri, Feb 08, 2019 at 08:51:24PM +0900, Masashi Honma wrote: > My purpose of this patch is not to catch exception if not needed. > Because catching exception makes debugging more difficult. > Indeed, sometimes I needed to comment out try statements temporarily > while doing python2 to 3 project. What is the benefit of not trying to catch the exception if it might never be raised on some python versions? I'm not sure how this would make debugging more difficult here. > Which do you like ? I have strong preference on not having to maintain duplicated code and also on not seeing sys.version_info being used anywhere unless it is absolutely necessary to do so for functionality or avoiding overly complex implementation. At least for me, this: try: err = vm[i]['proc'].stderr.read() if err != None: err = err.decode() vm[i]['err'] += err logger.debug("VM[%d] stderr.read[%s]" % (i, err)) except: pass looks much cleaner than this: if sys.version_info[0] > 2: err = vm[i]['proc'].stderr.read() if err != None: err = err.decode() vm[i]['err'] += err logger.debug("VM[%d] stderr.read[%s]" % (i, err)) else: try: err = vm[i]['proc'].stderr.read() vm[i]['err'] += err logger.debug("VM[%d] stderr.read[%s]" % (i, err)) except: pass
On 2019/02/08 23:07, Jouni Malinen wrote: > What is the benefit of not trying to catch the exception if it might > never be raised on some python versions? I'm not sure how this would > make debugging more difficult here. The try statement in vm_read_stdout() expects only "[Errno 11] Resource temporarily unavailable" exception. Then, I would like to write like this. By this code, I can recognize ENOMEM or other exceptions occurred. try: out = vm['proc'].stdout.read() if out == None: return False out = out.decode() except IOError as e: if e.errno == errno.EAGAIN: return False raise e I will send this part as RFC. > I have strong preference on not having to maintain duplicated code and > also on not seeing sys.version_info being used anywhere unless it is > absolutely necessary to do so for functionality or avoiding overly > complex implementation. > > At least for me, this: > > try: > err = vm[i]['proc'].stderr.read() > if err != None: > err = err.decode() > vm[i]['err'] += err > logger.debug("VM[%d] stderr.read[%s]" % (i, err)) > except: > pass > > looks much cleaner than this: > > if sys.version_info[0] > 2: > err = vm[i]['proc'].stderr.read() > if err != None: > err = err.decode() > vm[i]['err'] += err > logger.debug("VM[%d] stderr.read[%s]" % (i, err)) > else: > try: > err = vm[i]['proc'].stderr.read() > vm[i]['err'] += err > logger.debug("VM[%d] stderr.read[%s]" % (i, err)) > except: > pass > Ok, my previous patch was ugly indeed. I will rewrite. Masashi Honma.
diff --git a/tests/hwsim/vm/parallel-vm.py b/tests/hwsim/vm/parallel-vm.py index df6c19b16..e12d0dd92 100755 --- a/tests/hwsim/vm/parallel-vm.py +++ b/tests/hwsim/vm/parallel-vm.py @@ -89,10 +89,16 @@ def vm_read_stdout(vm, i): global rerun_failures ready = False - try: - out = vm['proc'].stdout.read().decode() - except: - return False + if sys.version_info[0] > 2: + out = vm['proc'].stdout.read() + if out == None: + return False + out = out.decode() + else: + try: + out = vm['proc'].stdout.read() + except: + return False logger.debug("VM[%d] stdout.read[%s]" % (i, out)) pending = vm['pending'] + out lines = [] @@ -190,12 +196,19 @@ def show_progress(scr): running = True first_running = True - try: - err = vm[i]['proc'].stderr.read().decode() - vm[i]['err'] += err - logger.debug("VM[%d] stderr.read[%s]" % (i, err)) - except: - pass + if sys.version_info[0] > 2: + err = vm[i]['proc'].stderr.read() + if err != None: + err = err.decode() + vm[i]['err'] += err + logger.debug("VM[%d] stderr.read[%s]" % (i, err)) + else: + try: + err = vm[i]['proc'].stderr.read() + vm[i]['err'] += err + logger.debug("VM[%d] stderr.read[%s]" % (i, err)) + except: + pass if vm_read_stdout(vm[i], i): scr.move(i + 1, 10) @@ -245,12 +258,19 @@ def show_progress(scr): continue running = True - try: + if sys.version_info[0] > 2: err = vm[i]['proc'].stderr.read() - vm[i]['err'] += err - logger.debug("VM[%d] stderr.read[%s]" % (i, err)) - except: - pass + if err != None: + err = err.decode() + vm[i]['err'] += err + logger.debug("VM[%d] stderr.read[%s]" % (i, err)) + else: + try: + err = vm[i]['proc'].stderr.read() + vm[i]['err'] += err + logger.debug("VM[%d] stderr.read[%s]" % (i, err)) + except: + pass ready = False if vm[i]['first_run_done']:
The result of reading non blocked empty stream is different between python2 and 3. The python2 sends "[Errno 11] Resource temporarily unavailable" exception. The python3 could read "None" without exception. So separate processing. Signed-off-by: Masashi Honma <masashi.honma@gmail.com> --- tests/hwsim/vm/parallel-vm.py | 50 ++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 15 deletions(-)