Subject: Re: AD review of draft-ietf-ipngwg-rfc2292bis-07.txt

From: Jack McCann <[email-masked]>
Date: Wed, 2 Oct 2002 16:29:50 -0400 (EDT)

Thomas Narten wrote:

>I'm asking about the second. I wonder, for example, whether someone
>familiar with the POSIX/Austin Group work has reviewed the document.
>My concern is that there may be some fairly trivial inconsistencies
>with this document and the basic API. It would be nice to try and fix
>those (if they exist). Can anyone speak to this point?

So I took a look at 2292bis-07.  Please note I am not speaking
on behalf of posix/austin group, but rather simply as someone
who has been witness to the standardization efforts there.
I reviewed primarily for alignment with 2553bis and the latest 
posix/austin standard.  I did not review, for example, correctness 
of constant values and data structures for match against IPv6 
protocol RFCs (hopefully there have already been enough eyes 
that have done that).

Some comments for alignment with the latest POSIX standard:

- I suggest you update the Posix references, using the same
  reference as 2553bis, which I show below (note the spec has
  now also been approved by ISO):

      IEEE Std. 1003.1-2001 Standard for Information Technology --
      Portable Operating System Interface (POSIX)

      Open Group Technical Standard: Base Specifications, Issue 6
      December 2001

      ISO/IEC 9945-1:2002, 9945-2:2002, 9945-3:2002, 9945-4:2002
      Information technology -- Portable Operating System
      Interface (POSIX) -- Parts 1, 2, 3 and 4

      http://www.opengroup.org/austin

- protocol family constants (PF_xxx) are not defined in this
  latest POSIX standard, replace all PF_xxx with AF_xxx
  (e.g. PF_INET -> AF_INET)

- section 21.1 shows msg_iovlen as type size_t,
  the latest POSIX defines msg_iovlen as type int

Some editorial comments:

- section 7.1 in this sentence, CMSG_LEN should be CMSG_SPACE (see
  the nice diagram on page 67, an "ancillary data object" includes
  the padding at the end)

   "When the application uses
    ancillary data it must pass the returned length to CMSG_LEN() to
    determine how much memory is needed for the ancillary data object
    (including the cmsghdr structure)."

- section 8 typo in first paragraph last sentence "Hob-by-Hop"

- section 10.5 inet6_opt_next, the statement "Typep points the option 
  type field" does not seem right, for typep to point to the option 
  type field, it would have to be passed as 'uint8_t **typep'; I think 
  you mean it points to a buffer into which the option type is stored,
  or using wording similar to lenp, "typep stores the option type"

- section 11.1 you should cite one or more references upon which
  this statement is based:

   "Also, path MTU discovery for multicast has severe scalability
    limitations and should thus be avoided by default."

- the data type usage in the example code could be cleaner, for example
  in section 22 page 72:

       int   extlen;
       int   cmsglen;

       extlen = inet6_rth_space(IPV6_RTHDR_TYPE_0, 3);
       cmsglen = CMSG_SPACE(extlen);

  inet6_rth_space() returns a size_t into int extlen.
  int extlen is passed to CMSG_SPACE which expects unsigned int.
  CMSG_SPACE returns unsigned int into int cmsglen.
  and so on.

A minor technical comment:

- There is an inconsistency in the inet6_opt_set_val and 
  inet6_opt_get_val functions.  In both functions, the offset 
  argument is type size_t, but the function return value (also
  an offset) is type int.  Given the intended usage of these
  functions, where the return value of one call can be used as
  the offset argument on the next call, the data type of the
  offset argument should be type int.

I also want to point out an issue that might be raised if this 
API is ever brought to the Austin group (IEEE/OpenGroup/ISO) for 
standardization.  The functions and macros listed below use a variety 
of data types for things that represent a "size", including int, 
unsigned int, and size_t.  All these items, or at least those of
type size_t, could instead be of type socklen_t.  Note that some 
of the items identified are for use with msg_controllen and cmsg_len,
which are type socklen_t.

Here is the rationale for socklen_t from the latest POSIX standard:

  The type socklen_t was invented to cover the range of
  implementations seen in the field.  The intent of socklen_t
  is to be the type for all lengths that are naturally bounded
  in size; that is, that they are the length of a buffer which
  cannot sensibly become of massive size: network addresses,
  host names, string representations of these, ancillary data,
  control messages, and socket options are examples.  Truly
  boundless sizes are represented by size_t as in read(),
  write(), and so on.

  All socklen_t types were originally (in BSD UNIX) of type int.
  During the development of IEEE Std 1003.1-2001, it was decided
  to change all buffer lengths to size_t, which appears at face
  value to make sense.  When dual mode 32/64-bit systems came along,
  this choice unnecessarily complicated system interfaces because
  size_t (with long) was a different size under ILP32 and LP64 models.
  Reverting to int would have happened except that some implementations
  had already shipped 64-bit-only interfaces.  The compromise was a
  type which could be defined to be any size by the implementation:
  socklen_t.

The following items could be of type socklen_t:

  - inet6_rth_space
    - return type

  - inet6_rth_init
    - bp_len argument

  - inet6_opt_init
    - return type (but what about error return?)
    - extlen argument

  - inet6_opt_append
    - return type (but what about error return?)
    - extlen, prevlen, len arguments

  - inet6_opt_finish
    - return type (but what about error return?)
    - extlen, prevlen arguments

  - inet6_opt_set_val
    - return type
    - offset, vallen arguments

  - inet6_opt_next
    - return type
    - extlen, prevlen, lenp arguments

  - inet6_opt_find
    - return type
    - extlen, prevlen, lenp arguments

  - inet6_opt_get_val
    - return type
    - offset, vallen arguments

  - CMSG_SPACE
    - return type could be socklen_t to match msg_controllen
    - argument type could also be socklen_t

  - CMSG_LEN
    - return type could be socklen_t to match cmsg_len type
    - argument type could also be socklen_t

Short of a changing all of these to socklen_t, we should at least
consider changing the occurances of the size_t type to either
socklen_t or int or unsigned int.  This would avoid the complications
raised by size_t on systems having to support both ILP32 and LP64 models.

- Jack

--------------------------------------------------------------------
IETF IPng Working Group Mailing List
IPng Home Page:                      http://playground.sun.com/ipng
FTP archive:                      ftp://playground.sun.com/pub/ipng
Direct all administrative requests to [email-masked]
--------------------------------------------------------------------

---

From: Jack McCann <[email-masked]>
Date: Thu, 3 Oct 2002 13:18:28 -0400 (EDT)

>raised by size_t on systems having to support both ILP32 and LP64 models.

Given that CMSG_SPACE and CMSG_LEN were defined in RFC 2292, and
that their definitions have not changed in 2292bis, and that they
do not used size_t, I'd vote for leaving them as is.

- Jack

---

From: JINMEI Tatuya / 神明達哉<[email-masked]>
Date: Sun, 06 Oct 2002 20:38:58 +0900

>>>>> On Wed, 2 Oct 2002 16:29:50 -0400 (EDT), 
>>>>> Jack McCann <[email-masked]> said:

> So I took a look at 2292bis-07.

Hello, thanks for the valuable comments.

>   now also been approved by ISO):

Thomas also asked the update.  I'll do that in the next rev.

>   (e.g. PF_INET -> AF_INET)

Yes, PF_xxx was already replaced with AF_xxx in my local copy.  The
fix will appear in the next rev.

>   the latest POSIX defines msg_iovlen as type int

OK.

>   the nice diagram on page 67, an "ancillary data object" includes

You're right.  Thanks for the correction.

> - section 8 typo in first paragraph last sentence "Hob-by-Hop"

Ditto.

>   or using wording similar to lenp, "typep stores the option type"

Ditto.  I'll change this part to "typep stores the option type".

>     limitations and should thus be avoided by default."

Hmm, I thought this was a kind of common sense these days, but there
seems to be no RFC talking about the scalability issue of multicast
path MTU discovery.  Does anyone in this list know a good reference?

> which are type socklen_t.

OK.  I'll check the entire draft and change length types accordingly.

Thanks,

					JINMEI, Tatuya
					Communication Platform Lab.
					Corporate R&D Center, Toshiba Corp.
					[email-masked]
--------------------------------------------------------------------
IETF IPng Working Group Mailing List
IPng Home Page:                      http://playground.sun.com/ipng
FTP archive:                      ftp://playground.sun.com/pub/ipng
Direct all administrative requests to [email-masked]
--------------------------------------------------------------------

---

From: JINMEI Tatuya / 神明達哉<[email-masked]>
Date: Tue, 15 Oct 2002 10:19:20 +0900

Hello,

I'm about to submit a new revision of the advanced API (rfc2292bis)
draft, mainly in response to comments from you two (thanks, again, for
the comments).  Most of the changes are trivial or based on consensus
in this list.  However, I'd like to make an explicit response to some
of the comments in order to make it sure that the changes have
addressed the comments.  If the changes still have some problems,
please make a quick (negative) ack.

A comment from Thomas:

>> int inet6_opt_init(void *extbuf, size_t extlen);
>> 
>> This function returns the number of bytes needed for the empty
>> extension header i.e. without any options.

> I don't understand this sentence. Does this then always return the
> same value? And doesn't an "empty" header require 0 bytes? Or is this
> actually "length remaining"?

> Actully, the "length" field is weird in subsequent usages to. Is it
> really a length, or an offset into the options? Calling it length is
> confusing. Is it too late to change this?

In the next revision Section 10.1 (for inet6_opt_init()) will have the
following paragraph:

   (Note: since the return value on success is based on a "constant"
   parameter, i.e. the empty extension header, an implementation may
   return a constant value.  However, this specification does not
   require the value be constant, and leaves it as implementation
   dependent.  The application should not assume a particular constant
   value as a successful return value of this function.)

Also, "prevlen" parameters in function prototypes for inet6_opt_xxx()
will be changed to "offset".

>     limitations and should thus be avoided by default."

I could not find a good reference for this sentence, but after
re-reading the entire section, I think the wording could have been
better without additional references.  I'd propose the following
paragraphs which replace the paragraph containing the sentence Jack
mentioned:

   [RFC-1981] describes how path MTU discovery works for multicast
   destinations.  From practice in using IPv4 multicast, however, many
   careless applications that send large multicast packets on the wire
   have caused implosion of ICMPv4 error messages.  The situation can be
   worse when there is a filtering node that blocks the ICMPv4 messages.
   Though the filtering issue applies to unicast as well, the impact is
   much larger in the multicast cases.

   Thus, applications sending multicast traffic should explicitly enable
   path MTU discovery only when they understand that the benefit of
   possibly larger MTU usage outweights the possible impact of MTU
   discovery for active sources across the delivery tree(s).  This
   default behavior is based on the today's practice with IPv4 multicast
   and path MTU discovery.  The behavior may change in the future once
   it is found that path MTU discovery effectively works with actual
   multicast applications and network configurations.

> which are type socklen_t.

In the next revision all size_t types will be changed to "socklen_t"
or "int".  The new type will be "socklen_t" whenever possible, but the
type of function return values will be "int" if it can be a negative
integer (as an error result).  If such return values are intended to
be used as input to other functions, the type of the corresponding
arguments will also be "int".  For example, the return type of
inet6_opt_init() and the 3rd argument of inet6_opt_append() will be
left as "int".

The return and argument types of CMSG_SPACE and CMSG_LEN will be
changed to socklen_t, considering the intended usage, in spite of the

					JINMEI, Tatuya