tahoe-lafs/trac-2024-07-25

MDMF: exception in unpack_checkstring being dropped #1540

New issue

Closed

opened 2011-09-23 21:14:18 +00:00 by davidsarah · 4 comments

davidsarah commented

2011-09-23 21:14:18 +00:00

Owner

Split from #1534.

davidsarah wrote:

layout.py defines:

def unpack_checkstring(checkstring):
    cs_len = struct.calcsize(PREFIX)
    version, seqnum, root_hash, IV = struct.unpack(PREFIX, checkstring[:cs_len])
    if version != 0: # TODO: just ignore the share
        raise UnknownVersionError("got mutable share version %d, but I only understand version 0" % version)
    return (seqnum, root_hash, IV)

but it is called by source:src/allmydata/mutable/publish.py@5231#L1105 in a context that doesn't seem to be specific to SDMF. Why doesn't that raise an UnknownVersionError?

kevan wrote:

The caller in publish handles the case in which the remote write fails (e.g., because of an uncoordinated write error, share corruption, or something else). It's possible that we don't have tests to exercise that, or that the exception is getting lost in the deferred chain somewhere. I'll take a look at it.
[...]
It seems that both of these contribute to the missing exception. http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1534/1534-dropped-error-and-tests.darcs.patch tweaks the control flow in the publisher a little bit so the exceptions don't get hidden, and then adds a test to exercise the code for MDMF files. UnknownVersionError still doesn't get raised, though, since the MDMF checkstring is shorter than the SDMF checkstring and causes struct.unpack to give up before the version can be extracted and checked. I'll work on a patch to make the new test pass.

Split from #1534. davidsarah wrote: > layout.py defines: ``` def unpack_checkstring(checkstring): cs_len = struct.calcsize(PREFIX) version, seqnum, root_hash, IV = struct.unpack(PREFIX, checkstring[:cs_len]) if version != 0: # TODO: just ignore the share raise UnknownVersionError("got mutable share version %d, but I only understand version 0" % version) return (seqnum, root_hash, IV) ``` > but it is called by source:src/allmydata/mutable/publish.py@5231#L1105 in a context that doesn't seem to be specific to SDMF. Why doesn't that raise an `UnknownVersionError`? kevan wrote: > The caller in publish handles the case in which the remote write fails (e.g., because of an uncoordinated write error, share corruption, or something else). It's possible that we don't have tests to exercise that, or that the exception is getting lost in the deferred chain somewhere. I'll take a look at it. [...] > It seems that both of these contribute to the missing exception. <http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1534/1534-dropped-error-and-tests.darcs.patch> tweaks the control flow in the publisher a little bit so the exceptions don't get hidden, and then adds a test to exercise the code for MDMF files. `UnknownVersionError` still doesn't get raised, though, since the MDMF checkstring is shorter than the SDMF checkstring and causes `struct.unpack` to give up before the version can be extracted and checked. I'll work on a patch to make the new test pass.

tahoe-lafs added the

labels 2011-09-23 21:14:18 +00:00

tahoe-lafs added this to the 1.9.0 milestone 2011-09-23 21:14:18 +00:00

david-sarah@jacaranda.org commented

2011-09-23 21:17:50 +00:00

Author

Owner

In changeset:4af626a798c3cfa9:

test_mutable.py: update SkipTest message for test_publish_surprise_mdmf to reference the right ticket number. refs #1540.

In changeset:4af626a798c3cfa9: ``` test_mutable.py: update SkipTest message for test_publish_surprise_mdmf to reference the right ticket number. refs #1540. ```

kevan commented

2011-09-25 00:45:20 +00:00

Author

Owner

Attachment fix-unpack-checkstring.darcs.patch (25801 bytes) added

break unpack_checkstring into unpack_sdmf_checkstring and unpack_mdmf_checkstring, misc. other cleanups

**Attachment** fix-unpack-checkstring.darcs.patch (25801 bytes) added break unpack_checkstring into unpack_sdmf_checkstring and unpack_mdmf_checkstring, misc. other cleanups

fix-unpack-checkstring.darcs.patch

25 KiB

kevan@isnotajoke.com commented

2011-09-25 04:56:01 +00:00

Author

Owner

In changeset:a911e15783e6fca7:

mutable/publish: use unpack_mdmf_checkstring and unpack_sdmf_checkstring instead of unpack_checkstring. fixes #1540

In changeset:a911e15783e6fca7: ``` mutable/publish: use unpack_mdmf_checkstring and unpack_sdmf_checkstring instead of unpack_checkstring. fixes #1540 ```

tahoe-lafs added the

fixed

label 2011-09-25 04:56:01 +00:00

kevan@isnotajoke.com closed this issue

2011-09-25 04:56:01 +00:00

david-sarah@jacaranda.org commented

2011-09-25 04:56:01 +00:00

Author

Owner

In changeset:c88adf0ac0e10000:

mutable/layout.py: make unpack_sdmf_checkstring and unpack_mdmf_checkstring more similar, and change an assert to give a more useful message if it fails. refs #1540

In changeset:c88adf0ac0e10000: ``` mutable/layout.py: make unpack_sdmf_checkstring and unpack_mdmf_checkstring more similar, and change an assert to give a more useful message if it fails. refs #1540 ```