Gotta run, but I hope these responses help. On 9 March 2015 at 10:13, Francois Le Faucheur (flefauch) <[email protected]> wrote: > Hi Martin, > > Getting onto you first batch of comments: > >> >> Summary: >> >> Major issues: >> >> Minor issues: >> >> S3.1 needs to define the basic atom that is used for the logging file. >> It appears as though the atom is an octet. > > We have the following statement: > " A CDNI Logging File MUST contain a sequence of lines containing US- > ASCII characters [CHAR_SET] terminated by CRLF.” > I see that as defining the atom as an ASCII character.
I'd prefer it be explicit near the ABNF definitions. >> S3.2 uses the wildcard ABNF rule (i.e., <text....>) too aggressively. > > Can you give me the specifics (unless this refers to what you've expanded > below already) Using <> says to a generic processor : ignore everything that follows. It breaks automatic validation. It's a cop-out that you don't need. You know what letters you want to permit, so build a rule for it. That does mean that you need to consider whether you want to permit the ASCII BELL (or BEL) character, but I think that you already have an answer ready-to-hand. >> RECLINE (a potentially confusing name) could be more strictly defined. > > potentially confusing, but hopefully not really given the consistent > structure across DIRLINE/DIRGROUP/RECLINE/RECGROUP. > >> >> <CDNI Logging File> = 1*<DIRGROUP RECGROUP> >> >> ... is not valid and could be: >> >> CDNI-LOG-FILE = 1*(DIRGROUP / RECGROUP) > > I have changed it to : > " > CDNI Logging File = 1*<DIRGROUP RECGROUP> > " Again, <> is not appropriate here, it puts what you have inside it down as comments, not actionable BNF. Also, that name is not valid ABNF. > This is to avoid defining an extra name (CDNI-LOG-FILE) and have to explain > that CDNI-LOG-FILE is actually the CDNI Logging File. > Let me know if that is not valid. > >> >> That implies that an empty file is not valid. I'd have thought that * >> rather than 1* would be better. > > A logging file with no Logging Records is considered valid and allowed, but a > logging file without any directive is not considered valid. > This is consistent with the text that I added based on our previous > discussion stating that “occurrences” for all directives need to be respected > (in other words, some directives are mandatory). I think that you could express that better than you have. LoggingFile = 1*(1*DIRGROUP *RECGROUP) Might work. >> S3.3 again uses wildcard rules too aggressively. DIRNAME (not the >> unix command) should be defined more explicitly, even if it is just to >> specify what characters are allowed. Use text to further constrain >> it. As it stands, a generic processor cannot know that the name >> doesn't permit the inclusion of (for instance) ":" or CRLF. A common >> method is to define something like this as: >> >> DIRNAME = Version-directive / UUID-directive / ... / extension-directive >> >> More commonly, go up a level and define directive using the choice. >> That way, each directive can define a name and value together using >> ABNF. e.g. >> >> Version-directive = "Version" ":" HTAB "CDNI" "/" 1*DIGIT "." 1*DIGIT >> > > I’ll park these ABNF comments since I got many others and will handle them > together. > > >> I wouldn't be so prescriptive about the version format. Unless there >> are specific semantics you want to extract from each digit. > > Fair enough. I have relaxed it and simply made the format NHTABSTRING. > > In the corresponding IANA section for the Version registry, I have added: > “ > Within the registry, Version values are to be allocated by IANA > according to the "Specification Required" policy specified in > [RFC5226]. Version values are to be allocated by IANA with a format > of NAMEFORMAT (see Section 3.1). Version values corresponding to > specifications produced by the IETF CDNI Working Group are to be > allocated a name starting with "CDNI". All other Version values are > to be allocated a name that does not start with "CDNI”. > “ Creating carve-outs in registries like that is inadvisable. Someone might attach semantics to them and then you get into trouble. See the RFC on x-. > I have kept CDNI/1.0 as the value allocated by this document. > > >> >> What is a receiver expected to do if the version *isn't* CDNI/1.0 ? >> Discard the entire file? That doesn't seem like a good way to ensure >> forward compatibility. > > This has been discussed and fixed already. > > >> >> UUID: this a Universally Unique IDentifier (UUID) >> from the UUID Uniform Resource Name (URN) namespace specified >> in [RFC4122]) for the CDNI Logging File. >> >> I don't know how to apply this. Is it the UUID portion of the URN or a URN? > > Good point. > > Text has been modified to: > “ > this a Uniform Resource Name (URN) from the > Universally Unique IDentifier (UUID) URN namespace specified in > [RFC4122]). The UUID contained in the URN uniquely identifies > the CDNI Logging File. > " > > >> >> Please don't use MUST here: If the >> two values are equal, then the received CDNI Logging File MUST >> be considered non-corrupted. > > Done. Replaced MUST by “is to be”. > >> >> MD5 collisions are easy to manufacture. I'd also drop the MUST from >> the following sentence. > > Done. Replaced MUST by “is to be”. > >> >> S3.4: more wildcards > > Sorry. I didn’t get that. <> rules, not () rules. <> is a wildcard. >> S3.4.1: I wouldn't worry about HTTP/2 being special here. All that >> speculation is painful, especially since HTTP/2 will likely be >> published before this document. Better to just cite it and avoid >> getting caught up in the details. > > The thing is that there are additional things in HTTP2 that could be logged > and the WG has not yet discussed whether there is real value in doing that, > and how. So we do not have specific extensions for those. So we need to be > clear that we “only” log the same thing for HTTP/2 as for HTTP/1.1, even if > we know that one may potentially want to log more. > And I’d rather we don;t hold publication of this document until teh WG has > worked out exactly waht to do for the few additional things specific to > HTTP/2. > So I think the current text is quite appropriate. If you aren't fixing it now, fix it later and don't worry about saying that you will. Because you might not. >> Nits/editorial comments: >> >> There are lots of uses of lowercase "may”. > > Right. But we’ve been quite careful at using “MAY" as per RFC2119 and “may" > where appropriate. > Do you see specific occurences of “may” that are not appropriate? Some people get antsy about lowercase uses because it is confusing. I don't. If you've checked I'm happy. Expect others to notice, that's all. >> Page 6 has a few instances of missing hyphens in dCDN3/dCDN2 etc… > > Fixed. > >> S6.2.1: chunck >> > > Fixed > >> S3.1 the DATE/TIME rules can/should reference RFC 3339 > > Good point. > > NEW: > “ > Dates are encoded as "full-date" specified in [RFC3339]. > “ > and > > NEW: > “ > Times are encoded as "partial-time" specified in [RFC3339]. > “ > >> >> S3.3 using ":" HTAB as a separator makes it hard to construct these >> files manually. > > This is a recommendation from ELF and commonly used today. Since we want to > be as consistent as possible with ELF, I’d rather we stick to HTAB. > >> >> S3.4 use of "c:" in combination with the unordered list is confusing >> when the prefix is actually "c-". Maybe use the string "c-" quotes >> and all to denote the prefix. >> > > Done. For example it says: > “ > "c-" refers to the User Agent that issues the request (corresponds to the > "client" of W3C Extended Log Format) > " > >> S3.4.1 citing RFC 7230 is sufficient for HTTP/1.1; and probably for >> HTTP in general at this moment. > > RFC3270 itself says that HTTP1.1 is specified collectively by RFC3270, > RFC3271,… RFC3275. So we may as well be consistent with that. We get this question all the time. You don't need to expand, because RFC 7230 does that for you... > Finally got to the bottom ! > > Thanks much for the review. > > Francois _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
