[AFS3-std] Re: New i-d draft-keiser-afs3-xdr-primitive-types-00

[Tom Keiser]

On Thu, Nov 10, 2011 at 6:12 PM, Andrew Deason <adeason@sinenomine.net> wro=
te:
> On Tue, 19 Jul 2011 18:10:38 -0500
> Tom Keiser <tkeiser@sinenomine.net> wrote:
>
>> Please see
>> http://tools.ietf.org/html/draft-keiser-afs3-xdr-primitive-types-00.
>> Comments are hereby solicited.
>
> Comments:
>
>>> 1. =A0Introduction
> [...]
>>> The Rx RPC protocol utilizes XDR [RFC4506] as its means of encoding
>>> RPC call and response payloads. =A0While XDR provides type definitions
>>> for each popular bit-length integer, it does so using relatively
>>> ambiguous names (e.g., hyper).
>

Hi Andrew,

Thank you for the detailed review.


> I don't think it's quite right to call the names "ambiguous", as the
> format, width, etc are explicitly defined in RFC4506. I think
> "confusing" makes more sense. I don't think this is deserving of a new
> revision or anything, though; but if you happen to cut a new one for
> other reasons anyway...
>

That's fair.  I suspect this will go through at least one more
revision, so I will adopt your wording.


>>> 4. =A0afsUUID
> [...]
>>> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>>> | =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0{3} =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0| =A0 =A0node[4] =A0 =A0|
>>> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>>> | =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0{3} =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0| =A0 =A0node[5] =A0 =A0|
>>> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>>>
>>> =A0 =A0{1} clock_seq_hi_and_reserved
>>> =A0 =A0{2} clock_seq_low
>>> =A0 =A0{3} sign extended padding: 0, or 0xffffff depending on value
>>> =A0 =A0 =A0 =A0of MSB in field to be sign-extended and padded
>
> Should there be some kind of explicit identification of which {3} fields
> correspond to the sign extension of which field? This is pretty
> intuitive by the rest of the document, but it is not explicit in this
> diagram.
>
> However, I think this separation unnecessarily complicates the spec; the
> diagram layout implicitly depends on XDR "internals" (that is, node[5]
> is on the right because XDR encodes with NBO, but we don't care since we
> offload the encoding to the XDR "int" type). I think it would be easier
> to just, for example, have the whole 32-bit row be node[5], but you just
> restrict node[5] to having values between 127 and -128 inclusive.
>

Ok.  I'll simplify this to just be a set of integer range constraints,
and otherwise discuss these fields as being sent over the wire as
32-bit wide XDR integers.

> See below:
>
>>> 4.1. =A0Encoding
> [...]
>>> =A0Many of the fields which are encoded in this data structure are
>>> =A0smaller than four octets. =A0In order to XDR encode these fields, th=
ey
>>> =A0must first be resized. =A0Since many of these fields are signed, thi=
s
>>> =A0involves sign extension. =A0This process depends upon the machine
>>> =A0architecture. =A0Virtually all machines in existence today utilize 2=
s-
>>> =A0complement integer arithmetic, where this process merely involves
>>> =A0padding with zeros if the MSB is zero or ones if the MSB is one.
>
> This seems to suggest that the encoding and decoding depends on the
> in-memory implementation of negative numbers of the machine doing the
> encoding and decoding, which is definitely inappropriate for a network
> protocol. I find this section a bit confusing, though; I don't see the
> need to really discuss sign-extension of the values, since the encoding
> of the individual fields is dictated by XDR. That is, if node[0] is -5,
> the algorithm for encoding is not "sign-extend -5 according to
> 2s-complement", but rather "give the XDR encoding routine a 32-bit
> integer of value -5".
>

Hmm.  That's a good point.  Dealing with how the microarchitecture
represents the integer is purely XDR's problem; I'll simplify the
language and the diagram to follow this line of reasoning.

Cheers,

-Tom