Draft:  draft-ietf-l2tpext-l2vpn-06.txt
Reviewer: Elwyn Davies [elwynd@dial.pipex.com]
Review Date:  Friday 2/3/2006 2:23 PM CST
IETF LC Date: 23 Jan 2006

Summary: This document is close to being ready for PS.  It has one minor issue 
relating to the M bit in the various AVPs (see below) and its 
readability/usefulness could be much improved by pointers to the 
relevant parts of RFC3931 for definitions and processes referred to.  It 
also could do with a terminology section to make it clear where all the 
various terms come from and expand some of the acronyms.


Minor issues:

M bit in AVPs:
==============
s4.3: In the description of the new AVPs, the M bit is specified as 
SHOULD be 0 for all three items i.e. the receiver should ignore the AVP 
if it doesn't understand it.  Two questions (one with two parts) come to 
mind:
- For each AVP, will everything work as expected if the sender includes 
the AVP and the receiver ignores it?  Should the behaviours in this 
eventuality be discussed?
- What circumstances would lead an implementer to set the M bit to 1?

A related point: in this application do any of the existing AVPs need to 
be sent with M=1?
 
At least a pointer to s5.2 of RFC3931 would help.

======================

Editorial:
s1: Acronyms PPP and LAC need expansion..  More generally a brief 
terminology section saying where terms are imported from (mostly either 
the L2TPv3 and L2VPN Framework documents) would be helpful.

s2, last para: s/cross-connect/cross-connects/

s4.1, para 1: s/to signaling L2VPNs/for signaling L2VPNs/, s/has some 
advantages/have some advantages/

s4.2: AVP, SCCRQ, SCCRP, ICRQ, ICRP, ICCN, SLI need expanding or (in the 
cased of the message IDs, noting that this is what they are and saying 
where they are defined).

s4.2: It would be worth noting that these existing AVPs are defined in 
s5.4.3 of RFC3931.

s4.3: All AVPs: should specify lengths are in octets.

s4.3: It would be useful to provide a pointer to s5.3 of RFC3931 for a 
description of hiding.

s4.3: MTU AVP should specify encoding (presumably integer, octet order?) 
of MTU.

s5.1, para 2: A pointer to the relevant IANA registry  for the  
psudowire types should be included and also a reference to 
draft-ietf-pwe3-iana-allocation-15.txt.

s5.1, para 4: s/forwarder identifier/forwarder identifiers/ (refers to 
both local and remote).

s5.2, last para:  A pointer to (presumably) to Session Mgmt Tie Breaker 
AVP in s5.4.4 of RFC3931 would help.

s5.3 (and also s1): Make it explicit that the object of the exercise is 
to create a complete mesh of interconnections between SOURCE_FORWARDERS 
and TARGET_FORWARDERS.

s5.3, algorithm: I know it is a bit pedantic but it should explicitly 
say start from beginning of SOURCE_FORWARDERS (new Step 0), and Step 1 
should say start again from the beginning of TARGET_FORWARDERS.  This 
should be matched in the diagram.

s6: Should point at the IANA registry procedures in RFC3931.

s7:  The text is rather clumsy.  Try something like: 'This specification 
does not introduce any additional security issues beyond those discussed 
in [RFC3931] and [L2VPN FW].'