Some docs in mutable/layout.py are slightly confusing.

[zancas]

I'm looking at layout.py. There're three (minor) points of confusion for me in the docs.

• 1: On (or about) line 27 the docs assert that the 16-byte random value of the read-key salt is stored as a set of 16 characters, but I believe that "characters" have context dependent size. So C-chars are 1 byte in size, as are some implementations of Python string elements which I tend to think of as "characters", but this is implementation dependent and not future proof, right?

• 2: On (or about) line 38 there's a reference to "padding" I haven't figured out what this padding is. Pointers and/or clarification appreciated.

• 3: On (or about) line 46 there's a reference to "this". If understand the line correctly "this" is "HEADER additions", but I'm only 83% confident. I vote "this" is replaced with its referee.

[zooko]

• Let's just avoid the word "character", which could mean something other than a byte in certain contexts (e.g. a unicode character), in favor of the word "byte", which never means anything else. :-)
• I don't understand ​this doc:

  The data length of the uploaded file. Modulo padding, this will be
  the same of the data length field. Like the data length field, it is
  an unsigned long long and can be quite large.
  

Maybe one of those places where it says "data length" it meant to say something else?

[davidsarah]

mutable/layout.py: improve confusing documentation of layout. fixes #1534

[davidsarah]

I'm confused by something else: the headers seem to be different for SDMF and MDMF, but the comment describes the SDMF headers without saying so. It should probably be a comment associated with SDMFSlotWriteProxy, except that there are other functions outside that class that only support SDMF.

Another thing; 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 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]

IIRC, that comment is a braindump from when I was learning about the SDMF share format, which is why it is specific to SDMF in places. +1 on attachment:fix-1534.darcs.patch​.

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.

[kevan]

control flow tweak and tests for inappropriate use of unpack_checkstring

[kevan]

Replying to kevan:

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. attachment:1534-dropped-error-and-tests.darcs.patch​ (or attachment:1354-dropped-error-and-tests.darcs.patch, if you can tolerate the typo :-) 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.

[davidsarah]

+1 on 1534-dropped-error-and-tests.darcs.patch​.

[david-sarah@…]

In 5d3d0dc33656c0ad:

test_mutable.py: skip test_publish_surprise_mdmf, which is causing an error. refs #1534, #393

[davidsarah]

The issue with unpack_checkstring has been split into #1540. This ticket now only deals with the documentation issue.

[warner]

what's the status of this? is the attached docs patch good enough?

[david-sarah@…]

In 87ca4fc7055faaea:

mutable/layout.py: improve confusing documentation of layout. fixes #1534