[quagga-dev 10330] Re: [PATCH] Enable the commands to support detailed debugging of LSAs.

Christian Franke chris at opensourcerouting.org
Fri Mar 8 20:17:22 GMT 2013


On 02/20/2013 04:57 PM, Dinesh Dutt wrote:
> The code for the commands exists, but it hasn't been defined in the
> definition of the command itself. This patch fixes that and also allows
> the definition via name instead of LSA type hex code.

I was wondering what the motivation behind this patch is because the
original implementation seemed to work for me. But now I realize that
you are probably not using telnet ::1 2606 but using vtysh and it does
not work there, right? (Funnly enough, only tab completion is broken,
the commands themselves will still work)

All in all I like the idea of generating commands from registered
handlers much better, however it seems that the current design of
vtysh/extract.pl does not really accommodate this at all.

-Christian

>  DEFUN (debug_ospf6_lsa_type,
>         debug_ospf6_lsa_hex_cmd,
> -       "debug ospf6 lsa XXXX/0xXXXX",
> +       "debug ospf6 lsa (router|network|inter-prefix|inter-router|as-ext|"
> +       "grp-mbr|type7|link|intra-prefix)",
>         DEBUG_STR
>         OSPF6_STR
>         "Debug Link State Advertisements (LSAs)\n"
> -       "Specify LS type as Hexadecimal\n"
> +       "Specify LSA type to debug\n"
>        )

This leaves us with a debug_ospf6_lsa_hex_cmd which doesn't do anything
with hex at all.

> +      if (strncmp (argv[0], ospf6_lsa_handler_name(handler), strlen(argv[0])) == 0)
>          break;
>        if (! strcasecmp (argv[0], handler->name))
>          break;

Having both of these checks in place seems somewhat moot. And they don't
cover every case. E.g. the handler for AS-external LSAs has name ==
"AS-External" while argv[0] will be "as-ext", so neither check will match.

> -  if (type && handler == NULL)
> +  if (handler == NULL)
>      {
>        handler = (struct ospf6_lsa_handler *)
>          malloc (sizeof (struct ospf6_lsa_handler));
>        memset (handler, 0, sizeof (struct ospf6_lsa_handler));
> -      handler->type = type;
> +      handler->type = OSPF6_LSTYPE_UNKNOWN;
>        handler->name = "Unknown";
>        handler->show = ospf6_unknown_lsa_show;
>        vector_set_index (ospf6_lsa_handler_vector,

This code path will be taken for "grp-mbr" and "type7" as there is
currently no handler registered for them. Overwriting the already
registered handler for unknown LSAs doesn't seem like the right action
for that case and will leak memory if done multiple times.

Either there should be handlers registered for every possible argument
so this codepath is never taken (and an error message is triggered
otherwise) or one does probably require the correct type number for the
given LSA type to do something sensible.




More information about the Quagga-dev mailing list