#1534 closed defect (fixed)

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

Reported by: zancas Owned by: davidsarah
Priority: minor Milestone: 1.9.2
Component: code-mutable Version: 1.9.0a2
Keywords: docs reviewed Cc: zooko
Launchpad Bug:

Description

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.

Attachments (2)

fix-1534.darcs.patch (66.2 KB) - added by davidsarah at 2011-09-14T14:44:53Z.
mutable/layout.py: improve confusing documentation of layout. fixes #1534
1534-dropped-error-and-tests.darcs.patch (20.2 KB) - added by kevan at 2011-09-18T01:05:01Z.
control flow tweak and tests for inappropriate use of unpack_checkstring

Download all attachments as: .zip

Change History (15)

comment:1 Changed at 2011-09-14T03:58:25Z by zooko

  • Owner changed from zooko to kevan
  • 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?

Changed at 2011-09-14T14:44:53Z by davidsarah

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

comment:2 Changed at 2011-09-14T14:49:10Z by davidsarah

  • Milestone changed from undecided to 1.9.0

comment:3 Changed at 2011-09-14T15:58:28Z by 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?

comment:4 follow-up: Changed at 2011-09-17T18:59:02Z by 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.

Changed at 2011-09-18T01:05:01Z by kevan

control flow tweak and tests for inappropriate use of unpack_checkstring

comment:5 in reply to: ↑ 4 Changed at 2011-09-18T01:20:34Z by 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 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.

[edit: remove reference to duplicate patch]

Last edited at 2011-09-18T17:47:31Z by davidsarah (previous) (diff)

comment:7 Changed at 2011-09-19T05:16:36Z by zooko

  • Keywords reviewed added; review-needed removed

comment:8 Changed at 2011-09-20T18:34:31Z by david-sarah@…

In 5d3d0dc33656c0ad:

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

comment:9 Changed at 2011-09-23T21:15:17Z by davidsarah

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

comment:10 Changed at 2011-10-28T04:46:58Z by warner

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

comment:11 Changed at 2012-02-24T06:03:02Z by zooko

  • Cc zooko added
  • Owner changed from kevan to zooko
  • Status changed from new to assigned
  • Version changed from 1.9.0a1 to 1.9.0a2

comment:12 Changed at 2012-04-01T00:34:22Z by davidsarah

  • Milestone changed from 1.11.0 to 1.9.2
  • Owner changed from zooko to davidsarah
  • Status changed from assigned to new

comment:13 Changed at 2012-04-01T01:11:46Z by david-sarah@…

  • Resolution set to fixed
  • Status changed from new to closed

In 87ca4fc7055faaea:

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

Note: See TracTickets for help on using tickets.