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

New Issue

zancas · 2011-09-14T00:22:57Z

zancas commented

2011-09-14 00:22:57 +00:00

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.

> 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.

zancas added the

labels 2011-09-14 00:22:57 +00:00

zancas added this to the undecided milestone 2011-09-14 00:22:57 +00:00

zooko was assigned by zancas

2011-09-14 00:22:57 +00:00

zooko commented

2011-09-14 03:58:25 +00:00

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?

* 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](http://tahoe-lafs.org/trac/tahoe-lafs/browser/trunk/src/allmydata/mutable/layout.py?annotate=blame&rev=5246#L38): ``` 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 commented

2011-09-14 14:44:53 +00:00

Attachment fix-1534.darcs.patch (67783 bytes) added

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

**Attachment** fix-1534.darcs.patch (67783 bytes) added mutable/layout.py: improve confusing documentation of layout. fixes #1534

fix-1534.darcs.patch

66 KiB

tahoe-lafs modified the milestone from undecided to 1.9.0

2011-09-14 14:49:10 +00:00

davidsarah commented

2011-09-14 15:58:28 +00:00

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 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?

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 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 commented

2011-09-17 18:59:02 +00:00

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.

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 commented

2011-09-18 01:05:01 +00:00

Attachment 1534-dropped-error-and-tests.darcs.patch (20672 bytes) added

control flow tweak and tests for inappropriate use of unpack_checkstring

**Attachment** 1534-dropped-error-and-tests.darcs.patch (20672 bytes) added control flow tweak and tests for inappropriate use of unpack_checkstring

1534-dropped-error-and-tests.darcs.patch

20 KiB

kevan commented

2011-09-18 01:20:34 +00:00

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. 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.

[remove reference to duplicate patch]edit:

Replying to [kevan](/tahoe-lafs/trac-2024-07-25/issues/1534#issuecomment-85642): > 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. [1534-dropped-error-and-tests.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-1d23-c8ed-f135-771c52002697) 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. [remove reference to duplicate patch]edit:

davidsarah commented

2011-09-18 17:50:58 +00:00

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

+1 on [1534-dropped-error-and-tests.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-1d23-c8ed-f135-771c52002697).

david-sarah@jacaranda.org commented

2011-09-20 18:34:31 +00:00

In changeset:5d3d0dc33656c0ad:

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

In changeset:5d3d0dc33656c0ad: ``` test_mutable.py: skip test_publish_surprise_mdmf, which is causing an error. refs #1534, #393 ```

davidsarah commented

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

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

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

warner commented

2011-10-28 04:46:58 +00:00

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

zooko added

1.9.0a2

and removed

1.9.0a1

labels 2012-02-24 06:03:02 +00:00

tahoe-lafs modified the milestone from 1.11.0 to 1.9.2

2012-04-01 00:34:22 +00:00

david-sarah@jacaranda.org commented

2012-04-01 01:11:46 +00:00

In changeset:87ca4fc7055faaea:

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

In changeset:87ca4fc7055faaea: ``` mutable/layout.py: improve confusing documentation of layout. fixes #1534 ```

tahoe-lafs added the

fixed

label 2012-04-01 01:11:46 +00:00

david-sarah@jacaranda.org closed this issue

2012-04-01 01:11:46 +00:00

Sign in to join this conversation.

No Label

LeastAuthority.com automation

code-frontend-ftp-sftp

code-frontend-magic-folder

somebody else's problem

No Milestone

No Assignees

4 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Reference: tahoe-lafs/trac-2024-07-25#1534