Hi Dale, Thanks for the review. I know Paul has some comments, but here are mine. On 01.10.18 04:42, Dale Worley wrote: > Reviewer: Dale Worley > Review result: Ready with Issues > > I am the assigned Gen-ART reviewer for this draft. The General Area > Review Team (Gen-ART) reviews all IETF documents being processed > by the IESG for the IETF Chair. Please treat these comments just > like any other last call comments. > > For more information, please see the FAQ at > > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. > > Document: draft-murchison-tzdist-tzif-14 > Reviewer: Dale R. Worley > Review Date: 2018-09-30 > IETF LC End Date: 2018-10-09 > IESG Telechat date: unknown > > Summary: > This draft is on the right track but has open issues, described > in the review. > > Major issues: > > 1. The draft does not specify the versioning and compatibility > principles governing TZif files and their processors. Certain > passages suggest that processors are expected to *not* examine the > version number in a file, but instead, extensions to the file format > are made by adding further data blocks to the end of the file, and > later-version processors handle earlier-version files by noticing that > latter data blocks are missing. This approach implies that processors > based on earlier versions to not notice that there appears to be > trailing garbage in the file. But silent acceptance of trailing > garbage is not specified, and this strategy is different from most > IETF standards. These files are self-contained, and may contain multiple versions of the same information for compatibility purposes. This is discussed in Section 4 of the document. I think it would be good to address your last sentence by adding a new second bullet point to that section, as follows: > o Implementations that only understand Version 1 MUST ignore any > data that extends beyond the calculated end of the version 1 data block. I think it also makes sense to modify the following as well. OLD: > o Implementations SHOULD calculate the total lengths of the 32- and > 64-bit headers and data blocks and compare them against the actual > file size, as part of a validity check for the file. > NEW: > o Implementations more recent than version 1 SHOULD calculate the > total lengths of the 32- and > ^^^^^^^^^^^^^^^^^^^^^^^ > 64-bit headers and data blocks and compare them against the actual > file size, as part of a validity check for the file. > If there is text that suggests that version information not be considered, could you please call that out? And it's worth noting that a vast amount of code out there operates in just this fashion today. > > 2. The semantics of the various data items -- what they mean and how > they are to be used in processing -- is only hinted at. I suspect > that the draft is targeted at members of the community who already > thoroughly understand the semantics of the data items (based on their > names), but this is not stated, nor is any reference given to where > one might learn this information. OTOH, the glossary (in section 2) > gives definitions of several terms that suggest the document is > attempting to define the semantics, but the definitions given are > nowhere near sufficient to specify those semantics. This isn't really actionable. Also, Section 2 is not in itself meant to provide comprehensive definition of all elements. Much of that is done in Section 3 for each data element. Thus I propose no change here. > > 3. Apparently (see comments on section 4), characters outside the > ASCII set are allowed in time zone designations. If so, their > encoding needs to be specified. This element is a reference to a POSIX string and must be handled in accordance with POSIX rules. This is already referenced. I'll leave it to Ken and Paul to address nits. Eliot > > Minor issues: > > Nits/editorial comments: > > > 1. Introduction > > This specification does not define the source of leap second > information, nor does it define the source the time zone data, > metadata, identifiers, aliases, localized names, or versions as > defined in Section 3 of [RFC7808]. One such source is the IANA- > hosted time zone database [RFC6557]. > > The sequence "nor does it define the source the time zone data" is > gramatically incorrect. I would suggest it should be "... the source > of ...", but making that change leaves the semantically heterogenous > list "the time zone data, metadata, identifiers, aliases, localized > names, or versions". Also, it's not clear how "as defined in Section > 3 of [RFC7808]" attaches to the sentence, since the closest item in > the list, "versions", is defined in section 3 and not RFC 7808. I > suspect that some part of the wording was accidentally deleted. > > 2. Conventions Used in This Document > > Daylight Saving Time (DST): The time according to a location's law > or practice, adjusted as necessary from standard time. The > adjustment may be positive, negative, or zero. > > This seems to read that "Daylight Saving Time" is not "standard time" > plus the summer time offset, but standard time, adjusted by whatever > summer time offset might be in effect at the moment. But that is not > not the definition of DST, at least, not as commonly used in the US. > What exactly is meant here? I would prefer a much more careful > explanation of how the various terms are used. > > 2. a change in whether standard or daylight saving time is in use > > Is this intended to include the regular transition between summer time > and winter time, or only when a locality starts or stops the practice > of using summer time each year or the schedule of transitions? > > UNIX Leap Time: [...] > Similarly, if > the second leap second record occurs at 1972-12-31 23:59:60 UTC, > its UNIX leap time would be 94694401; the second occurrence > accounts for the first leap second. > > I think this would be clearer as: > > Similarly, if the second leap second record occurs at 1972-12-31 > 23:59:60 UTC, the UNIX leap time of 1972-12-31 23:59:60 UTC > would be 94694401 and the UNIX leap time of 1973-01-01 00:00:00 > UTC would be 94694402. > > -- > > If a TZif file specifies no > leap second records, UNIX leap time is equivalent to UNIX time. > > I think s/equivalent to/equal to/ reads better here. > > Wall Time: The time as shown on a clock set according to a > location's law or practice. > > In what way does this differ from "Local Time"? > > After reading further in the document, I suspect that the structure > you are intending is: There is a "standard time" which is derived > from UTC, and a "daylight savings time" which is derived from UTC, > *both of which run all the time*. From those two, a "wall time" is > derived by specifying points where "wall time" switches between > "standard time" and "daylight savings time". > > 3. The Time Zone Information Format (TZif) > > The description here seems to be out of order. Also, the description > of the data blocks could be clearer; it seems to me that "containing" > makes more sense in this context than "using". A better ordering is: > > The time zone information format begins with a fixed 44-octet > header (Section 3.1). The TZif header contains a field which > specifies the version of the file's format. > > The header is followed by a variable-length data block (Section 3.2) > containing four-octet (32-bit) transition times and leap second > occurrences. These 32-bit values are limited to representing times > from 1901-12-13 20:45:52 through 2038-01-19 03:14:07 UT. > > Version 1 files terminate after the 32-bit data block. > Version 2 and 3 files extend the format by appending a second > 44-octet header, another variable-length data block containing eight-octet > (64-bit) transition times and leap second occurrences, and a variable > length footer (Section 3.3). These 64-bit values can represent times > approximately 292 billion years into the past or future. > > [Move "NOTE" to here.] > > Figure "General Format of TZif Files" > > I think the figure should use s/Transitions/Values/, since not all > of the values are transition times. > > 3.1. TZif Header > > +---------------+---+ > | magic (4) |ver| > +---------------+---+ > > s/ver/ver(1)/ > > '2' (0x32) Version 2 - The file MUST contain the 32-bit header > and data block, a 64-bit header and data block, and a footer. > > The phrases "32-bit header" and "64-bit header" don't work, as the > headers have the same format. They could be called "header for 32-bit > data", etc., but that's a bit awkward. Alternatively, names for all > of the data sections could be defined in section 2. > > typecnt: A four-octet unsigned integer specifying the number of > local time type records contained in the data block - MUST NOT be > zero. (Although time type 0 is not used in files that have > nonempty TZ strings but no transitions, it is nevertheless > required because many TZif readers reject files that lack time > types.) > > This is hard to understand. I *think* "time type 0" means "time type > record 0", or "the time type defined by by time type record 0", and > the parenthesized sentence means "In files that have nonempty TZ > strings but no transitions, the time type records are not used; > nonetheless, in this case there must be one record, time type 0 (which > will be ignored) because many TZif readers reject files that lack time > type records." Here, you are fighting against the very common usage > where a "record" of some sort has a "type" field which contains a > small integer, so you have to be very clear that the "0" *indexes* the > time type record, rather than being the value of a "type" field value > in the time type record. > > 3.2. TZif Data Block > > The TZif data block consists of seven variable-length elements, > [...] > > I think you want to say "A TZif data block", since there can be two of > them in a TZif file. > > In the initial data block, time values are 32-bit (TIME_SIZE = 4 > octets). > > I think it would read better to say "first data block", to parallel > "second data block" and also to clarify you are not referring to the > header. > > +---------------+-+-+---+ > | utoff (4) |dst|idx| > +---------------+---+---+ > > There is an extra "+" in the first line, and the field lengths should > be specified: > > +---------------+------+------+ > | utoff (4) |dst(1)|idx(1)| > +---------------+------+------+ > > -- > > (is)dst: A one-octet value indicating whether local time should > be considered Daylight Savings Time (DST). The value MUST be 0 > or 1. A value of one (1) indicates that DST is in effect. A > value of zero (0) indicates that standard time in effect. > > It seems to me to be better to phrase the last two sentences as "A > value of one (1) indicates that this time type is DST. A value of > zero (0) indicates that this time type is standard time." But that is > assuming that "time type" is not just describing a specific interval > of time, but rather a *type* of intervals of time. > > (desig)idx: A one-octet unsigned integer specifying an index into > the series of time zone designation octets, thereby selecting a > particular designation string. Each index MUST be in the range > [0, 'charcnt'), and MUST index a NUL-terminated (0x00) sequence > of octets. > > The last clause isn't very useful, as it is equivalent to "there must > be a NUL at or after the idx position in time zone designations". I > suspect the real meaning is "Each index MUST be in the range [0, > 'charcnt'), and is interpreted as designating the NUL-terminated > ASCII-encoded string of characters starting at position idx in the > time zone designations. (This string MAY be empty.) A NUL octet MUST > exist in the time zone designations at or after position idx." > > However, Appendix A interoperability item 9 implies that characters > outside the ASCII set are allowed. If so, their encoding needs to be > specified. > > 3.3. TZif Footer > > The footer diagram should be: > > +-------+--------------------+-------+ > | NL(1) | TZ string (0...) | NL(1) | > +-------+--------------------+-------+ > > 4. Interoperability Considerations > > o The sequence of time changes defined by the 32-bit header and data > block SHOULD be a contiguous subsequence of the time changes > defined by the 64-bit header and data block, and by the footer. > This guideline helps obsolescent version 1 readers agree with > current readers about timestamps within the contiguous > subsequence. It also lets writers not catering to obsolescent > readers use a 'timecnt' of zero in the 32-bit data to save space. > > I think s/not catering to/not supporting/, and s/32-bit data/32-bit > data block/. > > * "application/tzif-leap" (Section 8.2) to indicate that leap > second records are included in the TZif data. > > You probably need to spell out: "For version 1 files, leap second > records must be present in the 32-bit data block; for version 2 and 3 > files, leap second records must be present in the 64-bit data block." > > 10.3. URIs > > This section is odd. Item [1] is just the URL for BCP 14, but BCP 14 > is also listed in the references section. Items [6] and [7] duplicate > [8] and [9], but they're references and should be treated as such. > Items [3], [4], and [5] are more interesting -- they're locations > inside reference [POSIX]. The Editor should be able to suggest a > better way of presenting these references. > > Appendix A. Common Interoperability Issues > > Most of these are problems in generating TZif files > for use by readers conforming to predecessors of this specification. > > It would be helpful to those dealing with compatibility issues to have > references to the predecessors of this specification. > > > _______________________________________________ > Tzdist-bis mailing list > Tzdist-bis@xxxxxxxx > https://www.ietf.org/mailman/listinfo/tzdist-bis >
Attachment:
signature.asc
Description: OpenPGP digital signature
