parse_abbreviated_size doesn't accept T for terabytes (and other quibbles with the regex it uses)

[davidsarah]

stercor tried to use "reserved_space = 1.5TiB" in tahoe.cfg. This failed and used a reserved_space of 0 bytes (as shown on the server status page), because T is not recognized by parse_abbreviated_size in allmydata.util.abbreviate.

Note that the error due to an unparseable size is silent and only reported in the foolscap log. Continuing with a reserved_space of 0 bytes is a rather unhelpful failure mode. The code responsible for that is at git/src/allmydata/client.py@32377469#L248.

The regex is r"^(\d+)([kKmMgG]?[iB]?[bB]?)$", which also allows strings ending in "BB" or "Bb"; I don't think that allowing the repeated B was intentional.

[davidsarah]

Replying to davidsarah:

The regex is r"^(\d+)([kKmMgG]?[iB]?[bB]?)$", which also allows strings ending in "BB" or "Bb"; I don't think that allowing the repeated B was intentional.

... especially because parse_abbreviated_size will then fail with a KeyError (not a ValueError as it should) because only one B is stripped off, not both.

[davidsarah]

Please review: ​https://github.com/davidsarah/tahoe-lafs/commit/d8baf7aa5fe967f2611c55d297fb522b16706f89

Note that this only addresses the terabyte and double-B issues, not the error reporting. It also needs tests. I allowed E for exabytes as well.

[davidsarah]

Apparently I don't know my petabytes from my exabytes. Here's a fixed version: ​https://github.com/davidsarah/tahoe-lafs/commit/84d55da319f199c1ff335186330d34ebf94868fe

[davidsarah]

Review needed for ​https://github.com/tahoe-lafs/tahoe-lafs/pull/26 (now with tests).

[Zancas]

I don't understand why "I" and "i" are considered valid suffixes.

[Zancas]

The "^" symbol in the re.match call used to parse out the "number" and "suffix" values is, if I understand correctly, redundant with the way re.match works.

This is because the "pattern" (first) argument to re.match is applied to the beginning of the "string" (second) argument. The "^" specifies that the pattern is to match from the beginning of its target. So the two functions, the one intrinsic to re.match, and the one specified by "^" are identical.

This doesn't necessarily mean that the "^" should be removed from the code as it, debatably, increases readability.

[Zancas]

I've reviewed these changes and run the unittests they include.

I concur that the doubled "BB" suffix now correctly raises a ValueError.

I concur that Tera-, Peta-, and Exa-bytes are now correctly handled as scales of 2⁴⁰, 2⁵⁰, and 2⁶⁰ respectively.

I note that the "^" in the "pattern" argument to re.match is not strictly necessary, but assert that it is correct, and may enhance readability.

I do not understand why the suffix's "I" and "i" are to be handled as they are.

It seems to me that patterns that contain only "I", "i", or e.g. "iB", are errors, but it's plausible I'm simply unfamiliar with established convention.

[davidsarah]

Whether "IB" or "I" on its own should be an error is debatable, but it is currently accepted, and I don't see any particular reason to change that. Use of ^ at the start of the pattern is simply a matter of style; I prefer the explicitness. So I'll commit this as-is, plus a change to git/docs/configuration.rst to mention the new prefixes.

[davidsarah]

Change to configuration.rst is in eba64f2a, the rest is in the previous two patches.

[davidsarah]

Reopening to fix this:

Note that the error due to an unparseable size is silent and only reported in the foolscap log. Continuing with a reserved_space of 0 bytes is a rather unhelpful failure mode. The code responsible for that is at git/src/allmydata/client.py@32377469#L248.

[warner]

Looks like we should *not* catch the exception, and allow it to be raised, since init_storage() is called before daemonization, so errors will appear on the 'tahoe start' stderr. The only thing that might need special-casing is "reserved_space=" (i.e. the default do-not-reserve-any-space), just in case that returns an empty string or something unparseable. This should be covered in unit tests.

[zooko]

So this patch isn't actually ready, since it needs the improvements Brian described in comment:14. Removing the "reviewed" tag.

[davidsarah]

Replying to zooko:

So this patch isn't actually ready, since it needs the improvements Brian described in comment:14. Removing the "reviewed" tag.

The patch has already been applied, but when I reopened the ticket I should have removed the "reviewed" tag since it was not applicable to any yet-to-be-applied patch.

[zooko]

Um, so since the patch was applied, but according to Brian (comment:14), we should not catch the exception, does this mean that fixing the patch should be a blocker for Tahoe-LAFS v1.10? Assigning to Brian to clarify.

[warner]

Please see the last patch on ​https://github.com/warner/tahoe-lafs/tree/1812-parse-size for review, it should report the parse error early, and updates the tests to match.

[David-Sarah Hopwood <david-sarah@…>]

In b084396bddd1e12a:

client.py: throw error when reserved_space= is unparseable. Closes #1812.

This should now fail quickly (during "tahoe start"). Previously this
would silently treat an unparseable size as "0", and the only way to
discover that it had had a problem would be to look at the foolscap log,
or examine the storage-service web page for the unexpected "Reserved
Size" number.