Re: [Gen-art] [Tzdist-bis] Genart last call review of draft-murchison-tzdist-tzif-14

[Eliot Lear <[e-mail]>]

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
> [e-mail]
> https://www.ietf.org/mailman/listinfo/tzdist-bis
>