Properly handle out of order read fragments
This commit is contained in:
+11
-8
@@ -67,27 +67,30 @@ Reply payload (handle is zeroed; lengths describe this fragment):
|
|||||||
| 8 | `uint64` | `1` (read) |
|
| 8 | `uint64` | `1` (read) |
|
||||||
| 16 | `uint64` | Byte offset of the **request** |
|
| 16 | `uint64` | Byte offset of the **request** |
|
||||||
| 24 | `uint32` | Total request length |
|
| 24 | `uint32` | Total request length |
|
||||||
| 28 | `uint32` | Fragment index (`0`, `1`, …) |
|
| 28 | `uint32` | Byte offset of this fragment (`0`, `65536`, …) |
|
||||||
| 32 | `uint32` | This fragment’s byte length |
|
| 32 | `uint32` | This fragment’s byte length |
|
||||||
| 36 | `uint32` | Same as offset 32 |
|
| 36 | `uint32` | Same as offset 32 |
|
||||||
| 40 | `uint32` | `0` |
|
| 40 | `uint32` | `0` |
|
||||||
|
|
||||||
When there is a single fragment, offsets 24–31 look like a `uint64`
|
When there is a single fragment, offsets 24–31 look like a `uint64`
|
||||||
length (index is 0). The 129-sector capture shows why they are two
|
length (the fragment offset is 0). The 129-sector capture shows why
|
||||||
`uint32`s: fragment 0 has `(66048, 0)` then chunk 65536; fragment 1
|
they are two `uint32`s: fragment 0 has `(66048, 0)` then chunk 65536;
|
||||||
has `(66048, 1)` then chunk 512.
|
fragment 1 has `(66048, 65536)` then chunk 512. `0x00010000` at offset
|
||||||
|
28 is the byte offset, not a 0-based index.
|
||||||
|
|
||||||
Read loop: receive fragments with that `opId` until the concatenated
|
Read loop: receive fragments with that `opId` until the concatenated
|
||||||
data length equals the request. Use the `uint32` at payload offset 32
|
data length equals the request. Use the `uint32` at payload offset 32
|
||||||
as the extra-data size for that fragment. Do not treat extra data as
|
as the extra-data size for that fragment, and copy it to the byte
|
||||||
part of AIO `size` (that field stays 44).
|
offset at payload offset 28 — fragments are not always delivered in
|
||||||
|
order. Do not treat extra data as part of AIO `size` (that field stays
|
||||||
|
44).
|
||||||
|
|
||||||
129-sector example (one client request, two server fragments):
|
129-sector example (one client request, two server fragments):
|
||||||
|
|
||||||
```
|
```
|
||||||
C: type=7 opId=18 size=44 offset=0 length=66048
|
C: type=7 opId=18 size=44 offset=0 length=66048
|
||||||
S: type=7 opId=18 size=44 index=0 chunk=65536 + 65536 data
|
S: type=7 opId=18 size=44 dest=0 chunk=65536 + 65536 data
|
||||||
S: type=7 opId=18 size=44 index=1 chunk=512 + 512 data
|
S: type=7 opId=18 size=44 dest=65536 chunk=512 + 512 data
|
||||||
```
|
```
|
||||||
|
|
||||||
## Lab check
|
## Lab check
|
||||||
|
|||||||
@@ -227,7 +227,8 @@ What that comparison showed:
|
|||||||
VDDK repeats the byte length there.
|
VDDK repeats the byte length there.
|
||||||
- Zeros on the wire are real transferred zeros, not a sparse skip.
|
- Zeros on the wire are real transferred zeros, not a sparse skip.
|
||||||
|
|
||||||
Replay: `NfcDisk.read` loops on fragments until `length` bytes arrive.
|
Replay: `NfcDisk.read` places fragments at the byte offset in the
|
||||||
|
reply (they may arrive out of order) until `length` bytes are filled.
|
||||||
Proof: `tests/integration/test_nfc_read_write.py` writes a known pattern
|
Proof: `tests/integration/test_nfc_read_write.py` writes a known pattern
|
||||||
(including a 129-sector read that must assemble two fragments) and
|
(including a 129-sector read that must assemble two fragments) and
|
||||||
checks the bytes that came back.
|
checks the bytes that came back.
|
||||||
|
|||||||
@@ -198,7 +198,9 @@ class NfcDisk:
|
|||||||
|
|
||||||
Matches ``VixDiskLib_Read``: one ``NFC_AIO_MSG_IO`` request in
|
Matches ``VixDiskLib_Read``: one ``NFC_AIO_MSG_IO`` request in
|
||||||
byte units. If the length exceeds the AIO buffer (64 KiB) the
|
byte units. If the length exceeds the AIO buffer (64 KiB) the
|
||||||
server replies with several same-``opId`` fragments.
|
server replies with several same-``opId`` fragments, which are
|
||||||
|
placed by the fragment byte offset in the reply (they may arrive
|
||||||
|
out of order).
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
start_sector: Sector offset from the start of the disk.
|
start_sector: Sector offset from the start of the disk.
|
||||||
@@ -220,8 +222,10 @@ class NfcDisk:
|
|||||||
op_id = self._next_op_id()
|
op_id = self._next_op_id()
|
||||||
self._sock.sendall(
|
self._sock.sendall(
|
||||||
_pack_aio_hdr(NFC_AIO_MSG_IO, len(payload), op_id) + payload)
|
_pack_aio_hdr(NFC_AIO_MSG_IO, len(payload), op_id) + payload)
|
||||||
data = bytearray()
|
data = bytearray(length)
|
||||||
while len(data) < length:
|
filled = 0
|
||||||
|
seen: set[int] = set()
|
||||||
|
while filled < length:
|
||||||
rhdr = _recvn(self._sock, NFC_AIO_HDR_SIZE)
|
rhdr = _recvn(self._sock, NFC_AIO_HDR_SIZE)
|
||||||
rtype, rsize, rop = _unpack_aio_hdr(rhdr)
|
rtype, rsize, rop = _unpack_aio_hdr(rhdr)
|
||||||
if rtype != NFC_AIO_MSG_IO or rop != op_id:
|
if rtype != NFC_AIO_MSG_IO or rop != op_id:
|
||||||
@@ -232,13 +236,20 @@ class NfcDisk:
|
|||||||
if rsize < 36:
|
if rsize < 36:
|
||||||
raise NfcProtocolError(
|
raise NfcProtocolError(
|
||||||
f"AIO IO reply payload too short: {rsize}")
|
f"AIO IO reply payload too short: {rsize}")
|
||||||
chunk_len = struct.unpack_from("<I", body, 32)[0]
|
# Fragments may arrive out of order. Offset 28 is the byte
|
||||||
remaining = length - len(data)
|
# offset of this chunk within the request (0, 65536, …),
|
||||||
if chunk_len == 0 or chunk_len > remaining:
|
# not a 0-based index.
|
||||||
|
dest, chunk_len = struct.unpack_from("<II", body, 28)
|
||||||
|
if (
|
||||||
|
dest in seen
|
||||||
|
or chunk_len == 0
|
||||||
|
or dest + chunk_len > length):
|
||||||
raise NfcProtocolError(
|
raise NfcProtocolError(
|
||||||
f"AIO IO chunk length {chunk_len} invalid, "
|
f"AIO IO chunk offset={dest} length={chunk_len} invalid, "
|
||||||
f"remaining {remaining}")
|
f"request {length}")
|
||||||
data.extend(_recvn(self._sock, chunk_len))
|
seen.add(dest)
|
||||||
|
data[dest:dest + chunk_len] = _recvn(self._sock, chunk_len)
|
||||||
|
filled += chunk_len
|
||||||
return bytes(data)
|
return bytes(data)
|
||||||
|
|
||||||
def write(
|
def write(
|
||||||
|
|||||||
Reference in New Issue
Block a user