Merge pull request #32 from JPHutchins/fix/image-upload-write-respons…

…e-legacy

fix: a bug where legacy SMP servers' ImageUploadWriteResponse would n…

smp/image_management.py

@@ -85,12 +85,30 @@ class ImageUploadWriteResponse(message.WriteResponse):
     This is the offset of the next byte to be written. If the offset is equal to
     the length of the image, the upload is complete.
     """
+
     match: bool | None = None
     """Indicates if the uploaded data successfully matches the provided SHA256.
 
     Only sent in the final packet if CONFIG_IMG_ENABLE_IMAGE_CHECK is enabled.
     """
 
+    rc: int | None = None
+    """Legacy field that contains a return code; possibly `MGMT_ERR`.
+
+    This field may be present on old SMP server implementations or new SMP
+    server implementations that have set
+    `CONFIG_MCUMGR_SMP_LEGACY_RC_BEHAVIOUR=y` for backwards compatibility with
+    old SMP clients.
+
+    Note that we are not validating this field because we don't necessarily
+    trust the server to send us valid values. If this value is present, then it
+    indicates use of an SMP server that is out of spec and interpretation of the
+    value should be done with reference to that server's source code, rather
+    that the SMP specification.
+
+    Zephyr source code reference: https://github.com/zephyrproject-rtos/zephyr/blob/91a1e706535b2f99433280513c5bc66dfb918506/subsys/mgmt/mcumgr/grp/img_mgmt/src/img_mgmt.c#L397-L400
+    """  # noqa: E501
+
 
 class ImageEraseRequest(message.WriteRequest):
     _GROUP_ID = header.GroupId.IMAGE_MANAGEMENT

tests/test_image_management.py

@@ -244,8 +244,7 @@ def test_ImageUploadWriteRequest() -> None:
 
 @pytest.mark.parametrize("off", [None, 0, 1, 0xFFFF, 0xFFFFFFFF])
 @pytest.mark.parametrize("match", [None, True, False])
-@pytest.mark.parametrize("rc", [None, 0, 1, 10])
-def test_ImageUploadWriteResponse(off: int | None, match: bool | None, rc: int | None) -> None:
+def test_ImageUploadWriteResponse(off: int | None, match: bool | None) -> None:
     assert_header = make_assert_header(
         smpheader.GroupId.IMAGE_MANAGEMENT,
         smpheader.OP.WRITE_RSP,
@@ -280,3 +279,51 @@ def test_ImageUploadWriteResponse(off: int | None, match: bool | None, rc: int |
     assert_header(r)
     assert r.off == off
     assert r.match == match
+
+
+@pytest.mark.parametrize("off", [None, 0, 1, 0xFFFF, 0xFFFFFFFF])
+@pytest.mark.parametrize("match", [None, True, False])
+@pytest.mark.parametrize("rc", [None, 0, 1, -23478934])
+def test_legacy_ImageUploadWriteResponse(
+    off: int | None, match: bool | None, rc: int | None
+) -> None:
+    assert_header = make_assert_header(
+        smpheader.GroupId.IMAGE_MANAGEMENT,
+        smpheader.OP.WRITE_RSP,
+        smpheader.CommandId.ImageManagement.UPLOAD,
+        None,
+    )
+    r = smpimg.ImageUploadWriteResponse(off=off, match=match, rc=rc)
+
+    assert_header(r)
+    assert r.off == off
+    assert r.match == match
+    assert r.rc == rc
+
+    r = smpimg.ImageUploadWriteResponse.loads(r.BYTES)
+    assert_header(r)
+    assert r.off == off
+    assert r.match == match
+    assert r.rc == rc
+
+    if sys.version_info >= (3, 9):
+        cbor_dict = (
+            {}
+            | ({"off": off} if off is not None else {})
+            | ({"match": match} if match is not None else {})
+            | ({"rc": rc} if rc is not None else {})
+        )
+    else:
+        cbor_dict = {}
+        if off is not None:
+            cbor_dict["off"] = off
+        if match is not None:
+            cbor_dict["match"] = match
+        if rc is not None:
+            cbor_dict["rc"] = rc
+
+    r = smpimg.ImageUploadWriteResponse.load(r.header, cbor_dict)
+    assert_header(r)
+    assert r.off == off
+    assert r.match == match
+    assert r.rc == rc

tests/test_regressions.py

@@ -0,0 +1,34 @@
+"""This file is here to prevent specific regressions."""
+
+from typing import Final
+
+import pytest
+
+import smp.header as smphdr
+import smp.image_management as smpimg
+
+
+def test_smpclient_41() -> None:
+    """https://github.com/intercreate/smpclient/issues/41"""
+
+    RESPONSE: Final = b"\x03\x00\x00\x0c\x00\x01\x02\x01\xbfbrc\x00coff\x18\xa5\xff"
+    """A legacy `ImageUploadWriteResponse` that contains the rc field."""
+
+    r: Final = smpimg.ImageUploadWriteResponse.loads(RESPONSE)
+
+    assert r.header.op == smphdr.OP.WRITE_RSP
+    assert r.header.version == smphdr.Version.V1
+    assert r.header.flags == smphdr.Flag(0)
+    assert r.header.length == 12
+    assert r.header.group_id == smphdr.GroupId.IMAGE_MANAGEMENT
+    assert r.header.sequence == 2
+    assert r.header.command_id == smphdr.CommandId.ImageManagement.UPLOAD
+
+    assert r.off == 165
+    assert r.rc == 0
+
+    with pytest.raises(ValueError):
+        smpimg.ImageManagementErrorV1.loads(RESPONSE)
+
+    with pytest.raises(ValueError):
+        smpimg.ImageManagementErrorV2.loads(RESPONSE)