[ns] bugs in mpls/classifier-addr-mpls.cc ?

[Chris Develder <chris.develder@intec.rug.ac.be>]

Hi,

I'm using ns2.1b8 (system is irrelevant for this mail) -- file used for
diffs is $Header:
/nfs/jade/vint/PUBLIC_CVSROOT/ns-2/mpls/classifier-addr-mpls.cc,v 1.5
2001/03/06 20:53:41 haldar Exp $.

I suspect mpls/classifier-addr-mpls.cc contains at least 3 bugs (lines
1040, 1043 and 1070):
*** classifier-addr-mpls.cc.ORIG        Fri Sep 28 11:01:40 2001
--- classifier-addr-mpls.cc.DEBUGGED    Fri Sep 28 12:24:49 2001
***************
*** 1036,1047 ****
        if ((PFTnb < 0))
                PFTinsert(FEC,PHB, erLIBptr);
        else {
                if (LIBptr < 0)
!                       PFTupdate(PFTnb, LIBptr);
                else {
                        int i = LIBptr;
!                       while (i < 0) {
                                LIBptr = i;;
                                i = LIB_.Entry_[LIBptr].LIBptr_;
                        }
                        LIB_.Entry_[LIBptr].LIBptr_ = erLIBptr;
--- 1036,1047 ----
        if ((PFTnb < 0))
                PFTinsert(FEC,PHB, erLIBptr);
        else {
                if (LIBptr < 0)
!                       PFTupdate(PFTnb, erLIBptr);
                else {
                        int i = LIBptr;
!                       while (i >= 0) {
                                LIBptr = i;;
                                i = LIB_.Entry_[LIBptr].LIBptr_;
                        }
                        LIB_.Entry_[LIBptr].LIBptr_ = erLIBptr;
***************
*** 1066,1074 ****
                if (fLIBptr < 0)
                        PFTupdate(PFTnb, cLIBptr);
                else {
                        int i=fLIBptr;
!                       while (i < 0) {
                                fLIBptr = i;;
                                i = LIB_.Entry_[fLIBptr].LIBptr_;
                        }
                        LIB_.Entry_[fLIBptr].LIBptr_ = cLIBptr;
--- 1066,1074 ----
                if (fLIBptr < 0)
                        PFTupdate(PFTnb, cLIBptr);
                else {
                        int i=fLIBptr;
!                       while (i >= 0) {
                                fLIBptr = i;;
                                i = LIB_.Entry_[fLIBptr].LIBptr_;
                        }
                        LIB_.Entry_[fLIBptr].LIBptr_ = cLIBptr;

(original 'while (i < 0)' will never be true.)


2) With respect to the suspected bugs above: why do you want to stack
explicit routes on top of existing paths for the FEC (in ErLspBinding,
around line 1043), and why do you want to stack Coarse label on top of
already existing aggregations of Fine label in
MPLSAddressClassifier::FlowAggregation? Why don't you just do "if PFT
doesn't contain entry already, insert it, otherwise replace it"? IMHO, if
you really want to use stacking, you can use ErLspStacking for the ER-LSPs,
or multiple calls of FlowAggregation for aggregation of finer flows in a
succession of coarser ones. Therefore, I suggest following changes:
*** classifier-addr-mpls.cc.ORIG        Fri Sep 28 11:01:40 2001
--- classifier-addr-mpls.cc.MINE        Fri Sep 28 12:26:22 2001
***************
*** 1035,1052 ****
        int PFTnb = PFTlocate(FEC,PHB, LIBptr);
        if ((PFTnb < 0))
                PFTinsert(FEC,PHB, erLIBptr);
        else {
!               if (LIBptr < 0)
!                       PFTupdate(PFTnb, LIBptr);
!               else {
!                       int i = LIBptr;
!                       while (i < 0) {
!                               LIBptr = i;;
!                               i = LIB_.Entry_[LIBptr].LIBptr_;
!                       }
!                       LIB_.Entry_[LIBptr].LIBptr_ = erLIBptr;
!               }
        }
        return 0;
  }
 
--- 1035,1043 ----
        int PFTnb = PFTlocate(FEC,PHB, LIBptr);
        if ((PFTnb < 0))
                PFTinsert(FEC,PHB, erLIBptr);
        else {
!               PFTupdate(PFTnb, erLIBptr);
        }
        return 0;
  }
 
***************
*** 1062,1079 ****
        int PFTnb = PFTlocate(fineFEC,finePHB, fLIBptr);
        if ((PFTnb < 0))
                PFTinsert(fineFEC,finePHB, cLIBptr);
        else {
!               if (fLIBptr < 0)
!                       PFTupdate(PFTnb, cLIBptr);
!               else {
!                       int i=fLIBptr;
!                       while (i < 0) {
!                               fLIBptr = i;;
!                               i = LIB_.Entry_[fLIBptr].LIBptr_;
!                       }
!                       LIB_.Entry_[fLIBptr].LIBptr_ = cLIBptr;
!               }
        }
          return 0;
  }
 
--- 1053,1061 ----
        int PFTnb = PFTlocate(fineFEC,finePHB, fLIBptr);
        if ((PFTnb < 0))
                PFTinsert(fineFEC,finePHB, cLIBptr);
        else {
!               PFTupdate(PFTnb, cLIBptr);
        }
          return 0;
  }

A possible motivation for applying the proposed changes can be the
following: suppose that in node A, you explicitly install path P1 for fec F
at time t1. Suppose that at t2>t1, you explicitly install path P2 in node A
for fec F (with code as it is shipped in ns-2.1b8, you then stack label of
P2 onto label for P1). Suppose that sometime later, at t3 (>t2>t1), you
change your mind and want to install P1 again in node A for fec F. With
code as it is now, I guess you create some infinite loop: LIB pointer of
LIB-entry of P1 points to P2, and that of P2 to P1, so you would keep
stacking labels. Probably, this is not what you want... (Btw, I discovered
the bugs using a scenario doing just that: when running the script, I got
segfault and started looking at code.)

Kind regards,
Chris.