Re: [E1000-eedc] [PATCH 02/10] implementation of IEEE 802.1Qbg in lldpad, part 1

[Jens Osterkamp <jens@linux.vnet.ibm.com>]

On Thursday 23 September 2010, John Fastabend wrote:
> On 8/25/2010 5:27 AM, Jens Osterkamp wrote:
> > This patch contains the first part of an initial implementation of the
> > IEEE 802.1Qbg standard: it implements code for the exchange of EVB
> > capabilities between a host with virtual machines and an adjacent switch.
> > For this it adds a new EVB TLV to LLDP.
> > 
> > Exchange of EVB TLV may be enabled or disabled on a per port basis.
> > Information about the information negotiated by the protocol can be
> > queried on the commandline with lldptool.
> > 
> > This patch adds support for querying and setting parameters used in
> > the exchange of EVB TLV messages.
> > The parameters that can be set are:
> > 
> >         - forwarding mode
> >         - host protocol capabilities (RTE, ECP, VDP)
> >         - no. of supported VSIs
> >         - retransmission timer exponent (RTE)
> > 
> > The parameters are implemented as a local policy: all frames received by
> > an adjacent switch are validated against this policy and taken over where
> > appropriate. Negotiated parameters are stored in lldpads config, picked up
> > again and used at the next start.
> > 
> > The patch applies to lldpad 0.9.38 and still contains code to log protocol
> > activity more verbosely than it would be necessary in the final version.
> > 
> > Signed-off-by: Jens Osterkamp <jens@linux.vnet.ibm.com>

snip

> remove the unneeded braces in the if statements above.
> 
> > +       /* only look at the mode for now
> > +       if (tie->ccap == ed->tie->ccap) {
> > +               ed->tie->ccap = tie->ccap;
> > +       }
> > +
> > +       if (tie->cvsi == ed->tie->cvsi) {
> > +               ed->tie->cvsi = tie->cvsi;
> > +       }
> > +       */
> 
> Why is this commented out? I will review the spec shortly, but please
> remind me. If its not needed remove it and add it later in a clean
> patch

Its not used right now, so its commented out. I removed it for now.

> 
> > +
> > +       return TLV_OK;
> > +}
> > +
> > +/* evb_compare
> > + *
> > + * compare our own and received tlv_info_evb
> > + */
> > +static int evb_compare(struct evb_data *ed, struct tlv_info_evb *tie)
> > +{
> > +       printf("%s(%i): \n", __func__, __LINE__);
> > +
> > +       if ((ed->tie->cmode == tie->cmode) /* &&
> > +            (ed->tie->ccap == tie->ccap) &&
> > +           (ed->tie->cvsi == tie->cvsi)*/)
> 
> Same here if its going to be commented out lets remove it for now.

Removed it.

> 
> > +               return 0;
> > +       else
> > +               return 1;
> > +}
> > +
> > +/* evb_statemachine:
> > + *
> > + * handle possible states during EVB capabilities exchange
> > + *
> > + * possible states:    EVB_OFFER_CAPABILITIES
> > + *                     EVB_CONFIGURE
> > + *                     EVB_CONFIRMATION
> > + */
> > +static void evb_statemachine(struct evb_data *ed, struct tlv_info_evb *tie)

snip

> > diff --git a/lldptool.c b/lldptool.c
> > index 5cf2846..7e166fe 100644
> > --- a/lldptool.c
> > +++ b/lldptool.c
> > @@ -39,6 +39,7 @@
> >  #include "lldp_med_clif.h"
> >  #include "lldp_8023_clif.h"
> >  #include "lldp_dcbx_clif.h"
> > +#include "lldp_evb_clif.h"
> >  #include "lldptool.h"
> >  #include "lldptool_cli.h"
> >  #include "lldp_mod.h"
> > @@ -156,6 +157,7 @@ struct lldp_module *(*register_tlv_table[])(void) = {
> >         ieee8023_cli_register,
> >         med_cli_register,
> >         dcbx_cli_register,
> > +       evb_cli_register,
> >         NULL,
> >  };
> > 
> 
> Jens, in general this looks OK. Can you fix the coding style throughout
> I prefer for single line if statements,
> 
> if (x)
> 	a_line;
> else (y)
> 	b_line;
> 
> The intent here is to use the same coding style as linux kernel where it
> is reasonable to do so. Like I said before I know lldpad is not at all
> consistent in this regard, but I want to get the rest of the code in
> order and certainly have new code be consistent.

fixed this in all the places I could find.
I will address the rest your comments in my next posting of the series.

Thanks !

Jens


-- 

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294