From 437d4f83881a8a1ddc98ff2b26b2481412859965 Mon Sep 17 00:00:00 2001 From: Fiorentino Francesco Date: Thu, 31 Mar 2016 23:46:16 +0100 Subject: [PATCH 1/5] Bugfix for send() method; 'retry' parameter didn't work as specify by documentation Previously 'retry' parameter match the absolute number of resend before failing. With the fix 'retry' match the maximum number of resend a failed packet before failing --- xmodem/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/xmodem/__init__.py b/xmodem/__init__.py index 0ed7500..0fa231e 100644 --- a/xmodem/__init__.py +++ b/xmodem/__init__.py @@ -210,7 +210,7 @@ def send(self, stream, retry=16, timeout=60, quiet=False, callback=None): ''' Send a stream via the XMODEM protocol. - >>> stream = file('/etc/issue', 'rb') + >>> stream = open('/etc/issue', 'rb') >>> print(modem.send(stream)) True @@ -308,6 +308,7 @@ def callback(total_packets, success_count, error_count) success_count += 1 if callable(callback): callback(total_packets, success_count, error_count) + error_count = 0 break self.log.error('send error: expected ACK; got %r for block %d', @@ -370,7 +371,7 @@ def recv(self, stream, crc_mode=1, retry=16, timeout=60, delay=1, quiet=0): ''' Receive a stream via the XMODEM protocol. - >>> stream = file('/etc/issue', 'wb') + >>> stream = open('/etc/issue', 'wb') >>> print(modem.recv(stream)) 2342 From dc00598d40ddb8f7a2f1568d2aa0c0b795563b44 Mon Sep 17 00:00:00 2001 From: Francesco Fiorentino Date: Thu, 7 Apr 2016 23:09:38 +0100 Subject: [PATCH 2/5] Add unit test for retry parameter in send() method --- .gitignore | 2 + test/unit/test_xmodem.py | 98 +++++++++++++++++++++++++++++++++++----- 2 files changed, 88 insertions(+), 12 deletions(-) diff --git a/.gitignore b/.gitignore index ba04aac..b8fd992 100644 --- a/.gitignore +++ b/.gitignore @@ -6,4 +6,6 @@ /xmodem.egg-info /.coverage /.tox +/.cache +/.idea *.py.swp diff --git a/test/unit/test_xmodem.py b/test/unit/test_xmodem.py index 3b724d5..8127bbd 100644 --- a/test/unit/test_xmodem.py +++ b/test/unit/test_xmodem.py @@ -7,10 +7,13 @@ from io import BytesIO except ImportError: # python 2 - import StringIO.StringIO as BytesIO + from StringIO import StringIO as BytesIO # local -import xmodem +from xmodem import NAK +from xmodem import CRC +from xmodem import ACK +from xmodem import XMODEM # 3rd-party import pytest @@ -24,24 +27,95 @@ def dummy_putc(data, timeout=1): return 0 +def test_xmodem_bad_mode(): + # given, + mode = 'XXX' + modem = XMODEM(getc=dummy_getc, putc=dummy_putc, mode=mode) + # exercise + with pytest.raises(ValueError): + status = modem.send(BytesIO(b'dummy-stream')) + + @pytest.mark.parametrize('mode', ['xmodem', 'xmodem1k']) def test_xmodem_dummy_fails_send(mode): # given, - modem = xmodem.XMODEM(getc=dummy_getc, - putc=dummy_putc, - mode=mode) + modem = XMODEM(getc=dummy_getc, putc=dummy_putc, mode=mode) # exercise status = modem.send(BytesIO(b'dummy-stream')) # verify assert not status, ("Expected value of status `False'") -def test_xmodem_bad_mode(): +@pytest.mark.parametrize('mode', ['xmodem', 'xmodem1k']) +@pytest.mark.parametrize('stream_data', [BytesIO(b'dummy-stream ' * 17), + BytesIO(b'dummy-stream ' * 1000)]) +def test_xmodem_send_exceed_maximum_number_of_resend(mode, stream_data): + """ XMODEM has retry parameter this function ensure that xmodem.send() will + return False dude to the fact that resend exceeded """ # given, - mode = 'XXX' - modem = xmodem.XMODEM(getc=dummy_getc, - putc=dummy_putc, - mode=mode) + max_resend = 16 + + def generator(): + if mode == 'xmodem': + yield NAK + else: + yield CRC + + if mode == 'xmodem': + yield ACK + + for i in range(max_resend + 1): + yield None + + while True: + yield ACK + + mock = generator() + + def mock_getc(size, timeout=1): + try: + # python 2 + x = mock.next() + except AttributeError: + # python 3 + x = next(mock) + return x + + xmodem = XMODEM(getc=mock_getc, putc=dummy_putc, mode=mode) # exercise - with pytest.raises(ValueError): - status = modem.send(BytesIO(b'dummy-stream')) + assert not xmodem.send(stream=stream_data, retry=max_resend) + + +@pytest.mark.parametrize('mode', ['xmodem', 'xmodem1k']) +@pytest.mark.parametrize('stream_data', [BytesIO(b'dummy-stream ' * 17), + BytesIO(b'dummy-stream ' * 1000)]) +def test_xmodem_send_fails_once_each_packet(mode, stream_data): + """ XMODEM has retry parameter this test ensure that the number of retry for + the whole stream_data will be higher the max_resend pro packet """ + # given, + max_resend = 16 + + def generator(): + if mode == 'xmodem': + yield NAK + else: + yield CRC + + while True: + yield None + yield ACK + + mock = generator() + + def mock_getc(size, timeout=1): + try: + # python 2 + x = mock.next() + except AttributeError: + # python 3 + x = next(mock) + return x + + xmodem = XMODEM(getc=mock_getc, putc=dummy_putc, mode=mode) + # exercise + assert xmodem.send(stream=stream_data, retry=max_resend) From e1cc106f4564c1ddccad71d013db115f1af81a49 Mon Sep 17 00:00:00 2001 From: Jeff Quast Date: Thu, 14 Apr 2016 09:07:24 -0700 Subject: [PATCH 3/5] bugfix: 'retry >= error_count' off by 1 In many of these cases, 'error_count' is incremented before being compared to retry, so, where error_count is 0 and retry is 1, the expression: error_count += 1 if error_count >= retry: (...) would always evaluate, and never retry as intended --- xmodem/__init__.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/xmodem/__init__.py b/xmodem/__init__.py index 0fa231e..795e2ed 100644 --- a/xmodem/__init__.py +++ b/xmodem/__init__.py @@ -276,7 +276,7 @@ def callback(total_packets, success_count, error_count) 'got %r', char) error_count += 1 - if error_count >= retry: + if error_count > retry: self.log.info('send error: error_count reached %d, ' 'aborting.', retry) self.abort(timeout=timeout) @@ -316,7 +316,7 @@ def callback(total_packets, success_count, error_count) error_count += 1 if callable(callback): callback(total_packets, success_count, error_count) - if error_count >= retry: + if error_count > retry: # excessive amounts of retransmissions requested, # abort transfer self.log.error('send error: NAK received %d times, ' @@ -339,7 +339,7 @@ def callback(total_packets, success_count, error_count) else: self.log.error('send error: expected ACK; got %r', char) error_count += 1 - if error_count >= retry: + if error_count > retry: self.log.warn('EOT was not ACKd, aborting transfer') self.abort(timeout=timeout) return False @@ -467,7 +467,7 @@ def recv(self, stream, crc_mode=1, retry=16, timeout=60, delay=1, quiet=0): print(err_msg, file=sys.stderr) self.log.warn(err_msg) error_count += 1 - if error_count >= retry: + if error_count > retry: self.log.info('error_count reached %d, aborting.', retry) self.abort() From eb31b6a0a625a773aea43fc7a7b0d982cd944ab6 Mon Sep 17 00:00:00 2001 From: Jeff Quast Date: Thu, 14 Apr 2016 09:09:54 -0700 Subject: [PATCH 4/5] Adjust ferboz's tests: style & boundaries We should always chose to test on the edge of any numeric boundaries whenever they are identified as a given. In this case, by ensuring we always retry minimally 1 time, we discovered another bug (fixed by previous commit), where retry=1 would never retry, exposing an off-by-1 count. --- test/unit/test_xmodem.py | 57 +++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/test/unit/test_xmodem.py b/test/unit/test_xmodem.py index 8127bbd..125bc29 100644 --- a/test/unit/test_xmodem.py +++ b/test/unit/test_xmodem.py @@ -10,10 +10,7 @@ from StringIO import StringIO as BytesIO # local -from xmodem import NAK -from xmodem import CRC -from xmodem import ACK -from xmodem import XMODEM +from xmodem import NAK, CRC, ACK, XMODEM # 3rd-party import pytest @@ -50,15 +47,16 @@ def test_xmodem_dummy_fails_send(mode): @pytest.mark.parametrize('stream_data', [BytesIO(b'dummy-stream ' * 17), BytesIO(b'dummy-stream ' * 1000)]) def test_xmodem_send_exceed_maximum_number_of_resend(mode, stream_data): - """ XMODEM has retry parameter this function ensure that xmodem.send() will - return False dude to the fact that resend exceeded """ + """ Verify send(retry=n) after 'n' transfer failures of single block. """ + # given, - max_resend = 16 + max_resend = 3 - def generator(): + def getc_generator(): if mode == 'xmodem': yield NAK else: + # xmodem1k yield CRC if mode == 'xmodem': @@ -70,52 +68,51 @@ def generator(): while True: yield ACK - mock = generator() + mock = getc_generator() def mock_getc(size, timeout=1): - try: - # python 2 - x = mock.next() - except AttributeError: - # python 3 - x = next(mock) - return x + return next(mock) xmodem = XMODEM(getc=mock_getc, putc=dummy_putc, mode=mode) + # exercise - assert not xmodem.send(stream=stream_data, retry=max_resend) + result = xmodem.send(stream=stream_data, retry=max_resend) + + # verify + assert not result @pytest.mark.parametrize('mode', ['xmodem', 'xmodem1k']) @pytest.mark.parametrize('stream_data', [BytesIO(b'dummy-stream ' * 17), BytesIO(b'dummy-stream ' * 1000)]) def test_xmodem_send_fails_once_each_packet(mode, stream_data): - """ XMODEM has retry parameter this test ensure that the number of retry for - the whole stream_data will be higher the max_resend pro packet """ + """ Verify send(retry=n) under 'n' transfer failures of single block. """ # given, - max_resend = 16 + max_resend = 1 - def generator(): + def getc_generator(): if mode == 'xmodem': yield NAK else: + # xmodem1k yield CRC while True: + # fail yield None + + # succeed yield ACK - mock = generator() + mock = getc_generator() def mock_getc(size, timeout=1): - try: - # python 2 - x = mock.next() - except AttributeError: - # python 3 - x = next(mock) - return x + return next(mock) xmodem = XMODEM(getc=mock_getc, putc=dummy_putc, mode=mode) + # exercise - assert xmodem.send(stream=stream_data, retry=max_resend) + result = xmodem.send(stream=stream_data, retry=max_resend) + + # verify + assert result From c7ffd6d49e8cfb3d29bdb1d1d55b8c6191f3f252 Mon Sep 17 00:00:00 2001 From: Jeff Quast Date: Thu, 14 Apr 2016 09:13:50 -0700 Subject: [PATCH 5/5] Welcome to the next release, 0.4.4 --- README.rst | 12 +++++++++++- setup.py | 4 ++-- xmodem/__init__.py | 2 +- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/README.rst b/README.rst index d1fd59e..964fd78 100644 --- a/README.rst +++ b/README.rst @@ -44,6 +44,16 @@ For more information, take a look at the documentation_. Changes ======= +0.4.4: + * bugfix: Large file transfers in ``send()`` were more likely to fail for + small values of ``retry``: This value should be the maximum failures per + block transfer as documented, but was improperly implemented as the number + of failures allowed for the total duration of the transfer, `PR #21 + `_. + * bugfix: ``send(retry=n)`` and ``recv(retry=n)`` would not retry ``n-1`` + times, rather than ``n`` times as documented, causing 'retry=1' to never + retry, for example. + 0.4.3: * bugfix: ``putc()`` callback was called in series, 3 times for each part of xmodem block header, data, and checksum during block transfer. Now all @@ -57,7 +67,7 @@ Changes `Issue #16 `_. 0.4.1 - * bugfix: re-transmit in send() on NAK or timeout, previously + * bugfix: re-transmit in ``send()`` on ``NAK`` or timeout, previously re-transmissions (wrongly) occurred only on garbage bytes. `PR #12 `_. diff --git a/setup.py b/setup.py index eb6411a..b92edea 100644 --- a/setup.py +++ b/setup.py @@ -7,8 +7,8 @@ setup( name='xmodem', - version='0.4.3', - author='Wijnand Modderman, Jeff Quast', + version='0.4.4', + author='Wijnand Modderman, Jeff Quast, Kris Hardy', author_email='maze@pyth0n.org', description=('XMODEM protocol implementation.'), url='https://github.com/tehmaze/xmodem', diff --git a/xmodem/__init__.py b/xmodem/__init__.py index 795e2ed..4822bdc 100644 --- a/xmodem/__init__.py +++ b/xmodem/__init__.py @@ -112,7 +112,7 @@ __copyright__ = ['Copyright (c) 2010 Wijnand Modderman', 'Copyright (c) 1981 Chuck Forsberg'] __license__ = 'MIT' -__version__ = '0.4.3' +__version__ = '0.4.4' import platform import logging