Re: [yang-doctors] Yangdoctors early review of draft-ietf-teas-yang-te-types-01

["Mehmet Ersue" <[e-mail]>]

Cool. Many Thanks Jan!

Cheers,
Mehmet

> -----Original Message-----
> From: yang-doctors <[e-mail]> On Behalf Of Jan
> Lindblad
> Sent: Tuesday, October 30, 2018 2:57 PM
> To: [e-mail]
> Cc: [e-mail]; [e-mail];
[e-mail]
> Subject: [yang-doctors] Yangdoctors early review of
draft-ietf-teas-yang-te-
> types-01
> 
> Reviewer: Jan Lindblad
> Review result: Ready with Issues
> 
> This is my YANG Doctor early review of draft-ietf-teas-yang-te-types, or
> more specifically ietf-te-types.yang . I find the module ready with a
couple of
> issues. I have also noted a few nits. Generally speaking, I'm not sure if
> reviewing a -types module with plenty of groupings on its own is the best
> way to go about it. The groupings defined here may be used in good or not
> so good ways later.
> 
> General questions:
> 
> #1: There are many locations in the YANG talking about an "ERO subobject
> index"
> (and once RRO index, record route subobject). What is this, and how is it
> supposed to be used? The document is silent on this matter, and I have
seen
> modules with problems around numeric index leafs much like this earlier.
Are
> these numbers stable, i.e. remains the same forever?
> 
> #2: There are few leafs (5) with default values given, and none with
> mandatory.
> Probably needs to increase before we get to last call.
> 
> #3: There are several choices in the module that are meant to be augmented
> with additional cases. In many instances, this is explicitly spelled out,
very
> good.
> If this is meant to happen in all choices, it would be nice to point this
out in
> every instance. Also, if there are any specific assumptions or
considerations
> to keep in mind when augmenting in a new technology, please note that in
> the description as well.
> 
> Issues and nits:
> 
> #4: Unclear data type
> 419:
>   typedef admin-group {
>     type binary {
>       length 4;
> 
> What is the format of this binary? If this is always a 4-byte binary,
wouldn't a
> numeric type be more user friendly, e.g. uint32?
> 
> #5: identifier in container with optional leafs
> 1496:
>   grouping te-topology-identifier {
> 
> The name suggests this is used as an identifier, but all the leafs are
optional.
> This is not typical. They are also in a container, precluding them from
being
> used as list keys. Is that as intended?
> 
> #6: Optional -id leafs again
> 1700:
>           leaf node-id {
>           leaf link-tp-id {
> 1768:
>         leaf address {
> 1783:
>         leaf node-id {
>         leaf link-tp-id {
> 
> Leafs that appear to be used as identifiers are optional
> 
> #7: binary length in bits?
> 
> 1731:
>            leaf as-number {
>             type binary {
>               length 16;
> 1773:
>         leaf ip-flags {
>           type binary {
>             length 8;
> 1805:
>           leaf label-flags {
>             type binary {
>               length 8;
> 
> It appears to me the modeler might have thought the length is given in
bits.
> The value of length is in bytes, however.
> 
> #8: Must expression copy paste
> 1852:
>     container label-end {
>       must "not(../label-end/te-label/direction) or "
>         + "not(te-label/direction) "
>         + "or ../label-end/te-label/direction = te-label/direction" {
> 
> This must expression appears to have been copied from label-start. In any
> case, it always evaluates to true and has no effect.
> 
> #9: Unclear bit field
> 1885:
>     leaf range-bitmap {
>       type binary;
>       description
>         "When there are gaps between label-start and label-end,
>          this attribute is used to specify the positions
>          of the used labels.";
>     }
> 
> Need more information on how to interpret this leaf. Which bits map to
> what, and what does the bit field values 0 and 1 indicate?
> 
> #10: Canonical representation
> 67:
>   typedef te-bandwidth {
> 
> The type is based on a string with a pattern allowing hex characters and
an
> upper or lowercase P. Since the pattern allows multiple representations of
> the same underlaying value (0x1p10 presumably means the same as 0x1p0xa
> and
> 0x1P0XA) the question comes up if there is a canonical representation of
this
> value, e.g. using all lowercase and all hex, or if the string must be
> remembered exactly as given by the client. The description could answer
this
> question.
> 
> #11: Mix of upper and lowercase
> The module specifies many enumeration and identity values. Some are all
> lowercase. Some are all uppercase. The principle of least astonishment
> suggests to pick one and stick with it. YANG recommendations suggest to
use
> all lowercase when in doubt.
> 
>   typedef te-link-direction {
>   typedef te-label-direction {
>   typedef te-hop-type {
>   identity LSP_METRIC_TYPE {
>   identity LSP_METRIC_RELATIVE {
>   identity LSP_METRIC_ABSOLUTE {
>   identity LSP_METRIC_INHERITED {
> 
> Best Regards,
> /jan
> 
> 
> _______________________________________________
> yang-doctors mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/yang-doctors