#26541 closed defect (fixed)

Fix minor mistakes in the bandwidth-file dir-spec entry

Reported by: teor Owned by: juga
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-bwauth, torspec
Cc: atagar, juga Actual Points:
Parent ID: #3723 Points:
Reviewer: Sponsor:

Description (last modified by juga)

atagar pointed out the following errors in the bandwidth-file dir-spec entry:

The bandwidth-file line appears:

[At most once for votes; does not occur in consensuses.]

He also suggested that we could change the name to bandwidth-file-headers or bandwidth-headers if we want. I don't mind what we call it, but it has to match #3723.

I also noticed that the definition of Value is wrong, it should be:

Value ::= ArgumentCharValue+
ArgumentCharValue ::= any printing ASCII character except NL and SP.

We should also add the new file_created and latest_bandwidth header keywords from bandwidth-file-spec.

Edit: file_created and latest_bandwidth were added in #26155 and it has been merged 3 now to master
Edit: previous line is about adding them to dir-spec

Child Tickets

Change History (18)

comment:1 Changed 13 months ago by atagar

Thanks teor! I also suspect we might want to exclude '=' from the set of valid ArgumentCharValue, but no big whoop either way. :)

comment:2 in reply to:  1 Changed 13 months ago by teor

Replying to atagar:

Thanks teor! I also suspect we might want to exclude '=' from the set of valid ArgumentCharValue, but no big whoop either way. :)

No, that's a deliberate decision, to allow padded base64.

comment:3 Changed 13 months ago by juga

Description: modified (diff)

comment:4 Changed 13 months ago by juga

Description: modified (diff)

comment:5 Changed 13 months ago by juga

PR in: https://github.com/torproject/torspec/pull/18

To be totally congruent, bandwidth-file should be replaced by bandwidth-list-headers or bandwidth-list-file-headers, though i left it as bandwidth-file-headers

comment:6 Changed 13 months ago by atagar

Status: assignedmerge_ready

Thanks juga! Looks good to me.

comment:7 in reply to:  description ; Changed 13 months ago by teor

Status: merge_readyneeds_revision

Replying to teor:

The bandwidth-file line appears:

[At most once for votes; does not occur in consensuses.]

The patch has slightly different words. Please use the standard text here, so people can search for it:

[At most once for votes; does not occur in consensuses.

He also suggested that we could change the name to bandwidth-file-headers or bandwidth-headers if we want. I don't mind what we call it, but it has to match #3723.

I am happy with whatever you want to call it, but since the spec is bandwidth-file-spec.txt, bandwidth-file-headers is a good choice.

I also noticed that the definition of Value is wrong, it should be:

Value ::= ArgumentCharValue+
ArgumentCharValue ::= any printing ASCII character except NL and SP.

The definition is missing ArgumentCharValue, which does not appear anywhere else in dir-spec.txt:

ArgumentCharValue ::= any printing ASCII character except NL and SP.

We should also add the new file_created and latest_bandwidth header keywords from bandwidth-file-spec.

Edit: file_created and latest_bandwidth were added in #26155 and it has been merged 3 now to master
Edit: previous line is about adding them to dir-spec

In bandwidth-file-spec, the definitions of Timestamp and latest_bandwidth use exactly the same words:

of the most recent generator bandwidth result.

Can we use the same words in dir-spec, to avoid confusion?

People might wonder why timestamp= and latest_bandwidth= are the same.
So let's also include this part of the bandwidth-file-spec in dir-spec:

    This time MUST be identical to the timestamp.
    This duplicate value is included to make the format easier for people
    to read.

comment:8 Changed 13 months ago by atagar

The definition is missing ArgumentCharValue, which does not appear anywhere else in dir-spec.txt:

I'm not overly much a fan of the package line's spec but you could use the NONSPACE from it (that's what you described here). Personally I prefer more limited field definitions like shared-rand-commit's...

AlgName ::= 1*(ALPHA / DIGIT / "_" / "-")

But up to you on how broad you want the allowable values of these fields to be.

comment:9 in reply to:  8 Changed 13 months ago by teor

Replying to atagar:

The definition is missing ArgumentCharValue, which does not appear anywhere else in dir-spec.txt:

I'm not overly much a fan of the package line's spec but you could use the NONSPACE from it (that's what you described here). Personally I prefer more limited field definitions like shared-rand-commit's...

AlgName ::= 1*(ALPHA / DIGIT / "_" / "-")

But up to you on how broad you want the allowable values of these fields to be.

It's up to juga if they want to make this change.

At the moment, our sample data contains:

ArgumentCharValue ::= ALPHA / DIGIT / "_" / "-" / "$" / "." / "/" / ":" / "+"

https://gitweb.torproject.org/torspec.git/tree/bandwidth-file-spec.txt#n436

If we restrict the set of allowed characters:

  • we make it harder to add extra header lines, particularly if their values are copied from other systems, and
  • we make the code in #3723 more complicated.

comment:10 in reply to:  7 Changed 13 months ago by juga

Replying to teor:

Replying to teor:

The bandwidth-file line appears:

[At most once for votes; does not occur in consensuses.]

The patch has slightly different words. Please use the standard text here, so people can search for it:

[At most once for votes; does not occur in consensuses.

I looked quickly how this was expressed and found https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n2358, but i didn't see other parts where it is expressed exactly as you propose https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n1649.
So yeah, i'll change it to express it like that.

I also noticed that the definition of Value is wrong, it should be:

Value ::= ArgumentCharValue+
ArgumentCharValue ::= any printing ASCII character except NL and SP.

The definition is missing ArgumentCharValue, which does not appear anywhere else in dir-spec.txt:

ArgumentCharValue ::= any printing ASCII character except NL and SP.

In dir-spec, we have ArgumentChar defined in section 1.2, so i thought we could use that, as we re-use the term Keyword from there.
Actually i don't know now why i created ArgumentCharValue in bandwidth-file-spec, i could have reused ArgumentChar there too (if i'm not mistaken now).

comment:11 Changed 13 months ago by juga

(And i said in the commit in dir-spec i was using ArgumentChar, but by mistake i actually didn't)

comment:12 Changed 13 months ago by juga

Status: needs_revisionneeds_review

Modified according the comments. I used ArgumentChar, let me know if that's not correct.

comment:13 in reply to:  12 ; Changed 13 months ago by teor

Replying to juga:

Modified according the comments.

It looks great!

I used ArgumentChar, let me know if that's not correct.

One more fix - it needs to be ArgumentCharValue.

ArgumentChar includes space:

ArgumentChar ::= any printing ASCII character except NL.

ArgumentCharValue doesn't:

ArgumentCharValue ::= any printing ASCII character except NL and SP.

(Those names are confusing, aren't they? Maybe we should change ArgumentCharValue to ValueChar? We can open a separate ticket for that fix if you want.)

comment:14 Changed 13 months ago by teor

Status: needs_reviewneeds_revision

comment:15 in reply to:  13 Changed 13 months ago by juga

Status: needs_revisionneeds_review

Replying to teor:

I used ArgumentChar, let me know if that's not correct.

One more fix - it needs to be ArgumentCharValue.

ArgumentChar includes space:

ArgumentChar ::= any printing ASCII character except NL.

ArgumentCharValue doesn't:

ArgumentCharValue ::= any printing ASCII character except NL and SP.

oh, right, that was the reason why we introduced ArgumentCharValue, thanks for the pointer.

(Those names are confusing, aren't they? Maybe we should change ArgumentCharValue to ValueChar? We can open a separate ticket for that fix if you want.)

they are, just leaving that name for now

comment:16 in reply to:  5 Changed 13 months ago by teor

Replying to juga:

PR in: https://github.com/torproject/torspec/pull/18

Ok, this patch is ready to go!

It's on GitHub at https://github.com/juga0/torspec/tree/ticket26541

I splioff #26567 for the ArgumentCharValue fix. Someone can do it eventually.

comment:17 Changed 13 months ago by teor

Status: needs_reviewmerge_ready

comment:18 Changed 13 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.