Comments inline.
On Tue, Oct 20, 2015 at 5:51 PM, Alexis Fasquel <[email protected]> wrote:
> Now, concering the actual patch:
>
> I started implementing Paul’s suggestion, however I soon realised that it
> was not as simple as that.
> Since the command already have an optional parameter “*interval” *—although
> not define as optional but rather as a second command.
> Which means that if I was to add the optional “emrt” parameter, I would
> get an ambiguous command:
> - `dump bgp all /test 10`
> - `dump bgp all /test emrt`
>
> I could have just created another command hardcoding “emrt”, however
> considering the current state of the code, it did not appear as a good idea.
> I also could have group the two command within one (defining “*interval”*
> as an optional parameter) but it would have meant checking if the interval
> was not set as the literal “emrt” and act accordingly.
>
> The only way I found to have something clean was to add two new “bpp dump
> types” (alll-et/updates-et) —table dumps do not use the Extended Timestamp
> Header (see RFC) and I refactored everything so the implementation would
> only be two commands (dump and no dump). I updated the documentation to
> reflect those changes, both on the binary appendix and the dump commands
> —the latter were actually no well documented.
>
> I also updated “bgp_btoa.c” as Donald mentioned it —even though I could
> not find a compilation rule anywhere in the makefile. The only trace of
> this executable seems to be in the .gitignore…
>
> Finally, I removed 2 things from the code:
> - The struct pointer `struct thread *t_bgp_dump_routes` since it was
> apparently not use by anything.
> - The portion of code that would save to the config file `dump bp
> routes-mrt` commands without interval. This does not make any sense since
> it would only be a one time process when starting quagga (the routing table
> may not have been initialised yet). However, I might have overlook
> something?
>
> Once again, the patch can be found at the end of this email.
>
> I hope I did not miss anything and let me know what you think.
>
> Regards,
> Alexis
>
>
>
> diff --git a/bgpd/bgp_btoa.c b/bgpd/bgp_btoa.c
> index 7c70881..da1c759 100644
> --- a/bgpd/bgp_btoa.c
> +++ b/bgpd/bgp_btoa.c
> @@ -173,6 +173,8 @@ main (int argc, char **argv)
>
>
> if (type == MSG_PROTOCOL_BGP4MP)
> printf ("TYPE: BGP4MP");
> + else if (type == MSG_PROTOCOL_BGP4MP_ET)
> + printf ("TYPE: BGP4MP_ET");
> else if (type == MSG_TABLE_DUMP)
> printf ("TYPE: MSG_TABLE_DUMP");
> else
> diff --git a/bgpd/bgp_dump.c b/bgpd/bgp_dump.c
> index 3e2eda8..1523092 100644
> --- a/bgpd/bgp_dump.c
> +++ b/bgpd/bgp_dump.c
> @@ -37,10 +37,25 @@ Software Foundation, Inc., 59 Temple Place - Suite
> 330, Boston, MA
> enum bgp_dump_type
> {
> BGP_DUMP_ALL,
> + BGP_DUMP_ALL_ET,
> BGP_DUMP_UPDATES,
> + BGP_DUMP_UPDATES_ET,
> BGP_DUMP_ROUTES
> };
>
>
> +static const struct bgp_dump_type_map {
> + enum bgp_dump_type type;
> + const char *str;
> +} bgp_dump_type_map[] =
> + {
> + {BGP_DUMP_ALL, "all"},
> + {BGP_DUMP_ALL_ET, "all-et"},
> + {BGP_DUMP_UPDATES, "updates"},
> + {BGP_DUMP_UPDATES_ET, "updates-et"},
> + {BGP_DUMP_ROUTES, "routes-mrt"},
> + {0, NULL},
> + };
> +
> enum MRT_MSG_TYPES {
> MSG_NULL,
> MSG_START, /* sender is starting up */
> @@ -87,10 +102,6 @@ struct bgp_dump bgp_dump_updates;
> /* BGP dump structure for 'dump bgp routes' */
> struct bgp_dump bgp_dump_routes;
>
>
> -/* Dump whole BGP table is very heavy process. */
> -struct thread *t_bgp_dump_routes;
> -
> -/* Some define for BGP packet dump. */
> static FILE *
> bgp_dump_open_file (struct bgp_dump *bgp_dump)
> {
> @@ -173,17 +184,26 @@ bgp_dump_interval_add (struct bgp_dump *bgp_dump,
> int interval)
> static void
> bgp_dump_header (struct stream *obuf, int type, int subtype)
> {
> - time_t now;
> + struct timeval clock;
> + long msecs;
> + time_t secs;
>
>
> - /* Set header. */
> - time (&now);
> + gettimeofday(&clock, NULL);
> +
> + secs = clock.tv_sec;
> + msecs = clock.tv_usec;
>
>
> /* Put dump packet header. */
> - stream_putl (obuf, now);
> + stream_putl (obuf, secs);
> stream_putw (obuf, type);
> stream_putw (obuf, subtype);
> -
> stream_putl (obuf, 0); /* len */
> +
> + /* Adding microseconds for the MRT Extended Header */
> + if (type == MSG_PROTOCOL_BGP4MP_ET)
> + {
> + stream_putl (obuf, msecs);
> + }
> }
>
>
> static void
> @@ -394,15 +414,15 @@ bgp_dump_interval_func (struct thread *t)
> {
> /* In case of bgp_dump_routes, we need special route dump function.
> */
> if (bgp_dump->type == BGP_DUMP_ROUTES)
> - {
> - unsigned int seq = bgp_dump_routes_func (AFI_IP, 1, 0);
> + {
> + unsigned int seq = bgp_dump_routes_func (AFI_IP, 1, 0);
> #ifdef HAVE_IPV6
> - bgp_dump_routes_func (AFI_IP6, 0, seq);
> -#endif /* HAVE_IPV6 */
> - /* Close the file now. For a RIB dump there's no point in leaving
> - * it open until the next scheduled dump starts. */
> - fclose(bgp_dump->fp); bgp_dump->fp = NULL;
> - }
> + bgp_dump_routes_func (AFI_IP6, 0, seq);
> +#endif /* HAVE_IPV6 */
>
Now that we've gotten rid of the need to check for HAVE_IPV6 and that you
are in there, I would just remove the #ifdef guards.
> + /* Close the file now. For a RIB dump there's no point in leaving
> + * it open until the next scheduled dump starts. */
> + fclose(bgp_dump->fp); bgp_dump->fp = NULL;
> + }
> }
>
>
> /* if interval is set reschedule */
> @@ -474,7 +494,15 @@ bgp_dump_state (struct peer *peer, int status_old,
> int status_new)
> obuf = bgp_dump_obuf;
> stream_reset (obuf);
>
>
> - bgp_dump_header (obuf, MSG_PROTOCOL_BGP4MP, BGP4MP_STATE_CHANGE_AS4);
> + if (bgp_dump_all.type == BGP_DUMP_ALL_ET)
>
Why do we need to have this if statement? Why can't we just pass in
bgp_dump_all.type into bgp_dump_header and have it do the right 'thing'?
Yes I know the if statement moves into there, but why does bgp_dump_state
need to know about MSG_PROTOCOL_BGP4MP or _ET?
> + {
> + bgp_dump_header (obuf, MSG_PROTOCOL_BGP4MP_ET,
> BGP4MP_STATE_CHANGE_AS4);
> + }
> + else
> + {
> + bgp_dump_header (obuf, MSG_PROTOCOL_BGP4MP,
> BGP4MP_STATE_CHANGE_AS4);
> + }
> +
> bgp_dump_common (obuf, peer, 1);/* force this in as4speak*/
>
>
> stream_putw (obuf, status_old);
> @@ -502,14 +530,20 @@ bgp_dump_packet_func (struct bgp_dump *bgp_dump,
> struct peer *peer,
> obuf = bgp_dump_obuf;
> stream_reset (obuf);
>
>
> + int type = MSG_PROTOCOL_BGP4MP;
>
Please move this declaration to the function top.
> + if (bgp_dump->type == BGP_DUMP_UPDATES_ET || bgp_dump->type ==
> BGP_DUMP_ALL_ET)
> + {
> + type = MSG_PROTOCOL_BGP4MP_ET;
> + }
> +
>
Again, just pass in bgp_dump->type into bgp_dump_header and have it do the
if statement to figure out what to do. So far it will replace 2 if
statements with 1.
thanks!
donald
> /* Dump header and common part. */
> if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV) )
> {
> - bgp_dump_header (obuf, MSG_PROTOCOL_BGP4MP, BGP4MP_MESSAGE_AS4);
> + bgp_dump_header (obuf, type, BGP4MP_MESSAGE_AS4);
> }
> else
> {
> - bgp_dump_header (obuf, MSG_PROTOCOL_BGP4MP, BGP4MP_MESSAGE);
> + bgp_dump_header (obuf, type, BGP4MP_MESSAGE);
> }
> bgp_dump_common (obuf, peer, 0);
>
>
> @@ -586,21 +620,21 @@ bgp_dump_parse_time (const char *str)
> static int
> bgp_dump_unset (struct vty *vty, struct bgp_dump *bgp_dump)
> {
> - /* Set file name. */
> + /* Removing file name. */
> if (bgp_dump->filename)
> {
> free (bgp_dump->filename);
> bgp_dump->filename = NULL;
> }
>
>
> - /* This should be called when interval is expired. */
> + /* Closing file. */
> if (bgp_dump->fp)
> {
> fclose (bgp_dump->fp);
> bgp_dump->fp = NULL;
> }
>
>
> - /* Create interval thread. */
> + /* Removing interval thread. */
> if (bgp_dump->t_interval)
> {
> thread_cancel (bgp_dump->t_interval);
> @@ -609,6 +643,7 @@ bgp_dump_unset (struct vty *vty, struct bgp_dump
> *bgp_dump)
>
>
> bgp_dump->interval = 0;
>
>
> + /* Removing interval string. */
> if (bgp_dump->interval_str)
> {
> free (bgp_dump->interval_str);
> @@ -681,107 +716,63 @@ bgp_dump_set (struct vty *vty, struct bgp_dump
> *bgp_dump,
> return CMD_SUCCESS;
> }
>
>
> -
> DEFUN (dump_bgp_all,
> dump_bgp_all_cmd,
> - "dump bgp all PATH",
> - "Dump packet\n"
> - "BGP packet dump\n"
> - "Dump all BGP packets\n"
> - "Output filename\n")
> -{
> - return bgp_dump_set (vty, &bgp_dump_all, BGP_DUMP_ALL, argv[0], NULL);
> -}
> -
> -DEFUN (dump_bgp_all_interval,
> - dump_bgp_all_interval_cmd,
> - "dump bgp all PATH INTERVAL",
> + "dump bgp (all|all-et|updates|updates-et|routes-mrt) PATH
> [INTERVAL]",
> "Dump packet\n"
> "BGP packet dump\n"
> - "Dump all BGP packets\n"
> + "Dump all BGP packets\nDump all BGP packets (Extended Tiemstamp
> Header)\n"
> + "Dump BGP updates only\nDump BGP updates only (Extended Tiemstamp
> Header)\n"
> + "Dump whole BGP routing table\n"
> "Output filename\n"
> "Interval of output\n")
> {
> - return bgp_dump_set (vty, &bgp_dump_all, BGP_DUMP_ALL, argv[0],
> argv[1]);
> -}
> -
> -DEFUN (no_dump_bgp_all,
> - no_dump_bgp_all_cmd,
> - "no dump bgp all [PATH] [INTERVAL]",
> - NO_STR
> - "Dump packet\n"
> - "BGP packet dump\n"
> - "Dump all BGP packets\n")
> -{
> - return bgp_dump_unset (vty, &bgp_dump_all);
> -}
> -
> -DEFUN (dump_bgp_updates,
> - dump_bgp_updates_cmd,
> - "dump bgp updates PATH",
> - "Dump packet\n"
> - "BGP packet dump\n"
> - "Dump BGP updates only\n"
> - "Output filename\n")
> -{
> - return bgp_dump_set (vty, &bgp_dump_updates, BGP_DUMP_UPDATES, argv[0],
> NULL);
> -}
>
>
> -DEFUN (dump_bgp_updates_interval,
> - dump_bgp_updates_interval_cmd,
> - "dump bgp updates PATH INTERVAL",
> - "Dump packet\n"
> - "BGP packet dump\n"
> - "Dump BGP updates only\n"
> - "Output filename\n"
> - "Interval of output\n")
> -{
> - return bgp_dump_set (vty, &bgp_dump_updates, BGP_DUMP_UPDATES, argv[0],
> argv[1]);
> -}
> + int bgp_dump_type = 0;
> + const char *interval = NULL;
> + struct bgp_dump *bgp_dump_struct = NULL;
> + const struct bgp_dump_type_map *map = NULL;
>
>
> -DEFUN (no_dump_bgp_updates,
> - no_dump_bgp_updates_cmd,
> - "no dump bgp updates [PATH] [INTERVAL]",
> - NO_STR
> - "Dump packet\n"
> - "BGP packet dump\n"
> - "Dump BGP updates only\n")
> -{
> - return bgp_dump_unset (vty, &bgp_dump_updates);
> -}
> + for (map = bgp_dump_type_map; map->str; map++)
> + if (strcmp(argv[0], map->str) == 0)
> + bgp_dump_type = map->type;
>
>
> -DEFUN (dump_bgp_routes,
> - dump_bgp_routes_cmd,
> - "dump bgp routes-mrt PATH",
> - "Dump packet\n"
> - "BGP packet dump\n"
> - "Dump whole BGP routing table\n"
> - "Output filename\n")
> -{
> - return bgp_dump_set (vty, &bgp_dump_routes, BGP_DUMP_ROUTES, argv[0],
> NULL);
> -}
> + switch (bgp_dump_type)
> + {
> + case BGP_DUMP_ALL:
> + case BGP_DUMP_ALL_ET:
> + bgp_dump_struct = &bgp_dump_all;
> + break;
> + case BGP_DUMP_UPDATES:
> + case BGP_DUMP_UPDATES_ET:
> + bgp_dump_struct = &bgp_dump_updates;
> + break;
> + case BGP_DUMP_ROUTES:
> + default:
> + bgp_dump_struct = &bgp_dump_routes;
> + break;
> + }
>
>
> -DEFUN (dump_bgp_routes_interval,
> - dump_bgp_routes_interval_cmd,
> - "dump bgp routes-mrt PATH INTERVAL",
> - "Dump packet\n"
> - "BGP packet dump\n"
> - "Dump whole BGP routing table\n"
> - "Output filename\n"
> - "Interval of output\n")
> -{
> - return bgp_dump_set (vty, &bgp_dump_routes, BGP_DUMP_ROUTES, argv[0],
> argv[1]);
> + /* When an interval is given */
> + if (argc == 3)
> + {
> + interval = argv[2];
> + }
> + return bgp_dump_set (vty, bgp_dump_struct, bgp_dump_type,
> + argv[1], interval);
> }
>
>
> -DEFUN (no_dump_bgp_routes,
> - no_dump_bgp_routes_cmd,
> - "no dump bgp routes-mrt [PATH] [INTERVAL]",
> +DEFUN (no_dump_bgp_all,
> + no_dump_bgp_all_cmd,
> + "no dump bgp (all|updates|routes-mrt) [PATH] [INTERVAL]",
> NO_STR
> - "Dump packet\n"
> - "BGP packet dump\n"
> - "Dump whole BGP routing table\n")
> + "Stop dump packet\n"
> + "Stop BGP packet dump\n"
> + "Stop dump process all/all-et\n"
> + "Stop dump process updates/updates-et\n"
> + "Stop dump process route-mrt\n")
> {
> - return bgp_dump_unset (vty, &bgp_dump_routes);
> + return bgp_dump_unset (vty, &bgp_dump_all);
> }
>
>
> /* BGP node structure. */
> @@ -823,22 +814,30 @@ config_write_bgp_dump (struct vty *vty)
> {
> if (bgp_dump_all.filename)
> {
> + const char *type_str = "all";
> + if (bgp_dump_all.type == BGP_DUMP_ALL_ET)
> + type_str = "all-et";
> +
> if (bgp_dump_all.interval_str)
> - vty_out (vty, "dump bgp all %s %s%s",
> + vty_out (vty, "dump bgp %s %s %s%s", type_str,
> bgp_dump_all.filename, bgp_dump_all.interval_str,
> VTY_NEWLINE);
> else
> - vty_out (vty, "dump bgp all %s%s",
> + vty_out (vty, "dump bgp %s %s%s", type_str,
> bgp_dump_all.filename, VTY_NEWLINE);
> }
> if (bgp_dump_updates.filename)
> {
> + const char *type_str = "updates";
> + if (bgp_dump_updates.type == BGP_DUMP_UPDATES_ET)
> + type_str = "updates-et";
> +
> if (bgp_dump_updates.interval_str)
> - vty_out (vty, "dump bgp updates %s %s%s",
> + vty_out (vty, "dump bgp %s %s %s%s", type_str,
> bgp_dump_updates.filename, bgp_dump_updates.interval_str,
> VTY_NEWLINE);
> else
> - vty_out (vty, "dump bgp updates %s%s",
> + vty_out (vty, "dump bgp %s %s%s", type_str,
> bgp_dump_updates.filename, VTY_NEWLINE);
> }
> if (bgp_dump_routes.filename)
> @@ -847,9 +846,6 @@ config_write_bgp_dump (struct vty *vty)
> vty_out (vty, "dump bgp routes-mrt %s %s%s",
> bgp_dump_routes.filename, bgp_dump_routes.interval_str,
> VTY_NEWLINE);
> - else
> - vty_out (vty, "dump bgp routes-mrt %s%s",
> - bgp_dump_routes.filename, VTY_NEWLINE);
> }
> return 0;
> }
> @@ -868,14 +864,7 @@ bgp_dump_init (void)
> install_node (&bgp_dump_node, config_write_bgp_dump);
>
>
> install_element (CONFIG_NODE, &dump_bgp_all_cmd);
> - install_element (CONFIG_NODE, &dump_bgp_all_interval_cmd);
> install_element (CONFIG_NODE, &no_dump_bgp_all_cmd);
> - install_element (CONFIG_NODE, &dump_bgp_updates_cmd);
> - install_element (CONFIG_NODE, &dump_bgp_updates_interval_cmd);
> - install_element (CONFIG_NODE, &no_dump_bgp_updates_cmd);
> - install_element (CONFIG_NODE, &dump_bgp_routes_cmd);
> - install_element (CONFIG_NODE, &dump_bgp_routes_interval_cmd);
> - install_element (CONFIG_NODE, &no_dump_bgp_routes_cmd);
> }
>
>
> void
> diff --git a/bgpd/bgp_dump.h b/bgpd/bgp_dump.h
> index e097c78..ab8e4eb 100644
> --- a/bgpd/bgp_dump.h
> +++ b/bgpd/bgp_dump.h
> @@ -24,6 +24,7 @@ Software Foundation, Inc., 59 Temple Place - Suite 330,
> Boston, MA
> /* MRT compatible packet dump values. */
> /* type value */
> #define MSG_PROTOCOL_BGP4MP 16
> +#define MSG_PROTOCOL_BGP4MP_ET 17
> /* subtype value */
> #define BGP4MP_STATE_CHANGE 0
> #define BGP4MP_MESSAGE 1
> diff --git a/doc/appendix.texi b/doc/appendix.texi
> index 87276c5..3395363 100644
> --- a/doc/appendix.texi
> +++ b/doc/appendix.texi
> @@ -30,6 +30,26 @@ don't need to change header format.
> @end group
> @end example
>
>
> + If `type' is PROTOCOL_BGP4MP_ET, the common header format will
> +contain an additional microsecond field (RFC6396 2011).
> +
> +@example
> +@group
> +0 1 2 3
> +0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> +| Time |
> ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> +| Type | Subtype |
> ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> +| Length |
> ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> +| Microsecond |
> ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> +@end group
> +@end example
> +
> +
> If `type' is PROTOCOL_BGP4MP, `subtype' is BGP4MP_STATE_CHANGE, and
> Address Family == IP (version 4)
>
>
> @@ -228,6 +248,7 @@ If `type' is PROTOCOL_BGP4MP and `subtype' is
> BGP4MP_SNAPSHOT,
> Constants:
> /* type value */
> #define MSG_PROTOCOL_BGP4MP 16
> + #define MSG_PROTOCOL_BGP4MP_ET 17
> /* subtype value */
> #define BGP4MP_STATE_CHANGE 0
> #define BGP4MP_MESSAGE 1
> diff --git a/doc/bgpd.texi b/doc/bgpd.texi
> index 7d92b5e..dd3e860 100644
> --- a/doc/bgpd.texi
> +++ b/doc/bgpd.texi
> @@ -1326,21 +1326,40 @@ log file bgpd.log
> @node Dump BGP packets and table
> @section Dump BGP packets and table
>
>
> -@deffn Command {dump bgp all @var{path}} {}
> -@deffnx Command {dump bgp all @var{path} @var{interval}} {}
> +
> +@deffn Command {dump bgp all @var{path} [@var{interval}]} {}
> +@deffnx Command {dump bgp all-et @var{path} [@var{interval}]} {}
> +@deffnx Command {no dump bgp all [@var{path}] [@var{interval}]} {}
> Dump all BGP packet and events to @var{path} file.
> +If @var{interval} is set, a new file will be created for echo
> @var{interval} of seconds.
> +The path @var{path} can be set with date and time formatting (strftime).
> +The type ‘all-et’ enables support for Extended Timestamp Header
> (@pxref{Packet Binary Dump Format}).
> +(@pxref{Packet Binary Dump Format})
> +
> @end deffn
>
>
> -@deffn Command {dump bgp updates @var{path}} {}
> -@deffnx Command {dump bgp updates @var{path} @var{interval}} {}
> -Dump BGP updates to @var{path} file.
> +@deffn Command {dump bgp updates @var{path} [@var{interval}]} {}
> +@deffnx Command {dump bgp updates-et @var{path} [@var{interval}]} {}
> +@deffnx Command {no dump bgp updates [@var{path}] [@var{interval}]} {}
> +Dump only BGP updates messages to @var{path} file.
> +If @var{interval} is set, a new file will be created for echo
> @var{interval} of seconds.
> +The path @var{path} can be set with date and time formatting (strftime).
> +The type ‘updates-et’ enables support for Extended Timestamp Header
> (@pxref{Packet Binary Dump Format}).
> @end deffn
>
>
> -@deffn Command {dump bgp routes @var{path}} {}
> -@deffnx Command {dump bgp routes @var{path}} {}
> +
> +@deffn Command {dump bgp routes-mrt @var{path}} {}
> +@deffnx Command {dump bgp routes-mrt @var{path} @var{interval}} {}
> +@deffnx Command {no dump bgp route-mrt [@var{path}] [@var{interval}]} {}
> Dump whole BGP routing table to @var{path}. This is heavy process.
> +The path @var{path} can be set with date and time formatting (strftime).
> +If @var{interval} is set, a new file will be created for echo
> @var{interval} of seconds.
> +
> @end deffn
>
>
> +Note: the interval variable can also be set using hours and minutes:
> 04h20m00.
> +
> +
> @node BGP Configuration Examples
> @section BGP Configuration Examples
>
>
>
> On 20 Oct 2015, at 14:48, Alexis Fasquel <[email protected]> wrote:
>
>
> Hello Paul, Donald,
>
> Let me first share a bug that I found while working on the Extended MRT
> Header patch.
>
> So basically what happen is the dumping thread are not cancelled when
> defining a new “dump rule” which can eventually leads to segmentation fault:
>
> `dump bgp all /test 5`
> `dump bgp all /test2 5`
> `no dump bgp all`
> -> Wait for a few seconds… and boom!
>
> Here’s bellow the fix for that issue.
>
> Regards,
> Alexis
>
> diff --git a/bgpd/bgp_dump.c b/bgpd/bgp_dump.c
> index a3c9526..3e2eda8 100644
> --- a/bgpd/bgp_dump.c
> +++ b/bgpd/bgp_dump.c
> @@ -584,61 +584,6 @@ bgp_dump_parse_time (const char *str)
> }
>
> static int
> -bgp_dump_set (struct vty *vty, struct bgp_dump *bgp_dump,
> - enum bgp_dump_type type, const char *path,
> - const char *interval_str)
> -{
> - unsigned int interval;
> -
> - if (interval_str)
> - {
> -
> - /* Check interval string. */
> - interval = bgp_dump_parse_time (interval_str);
> - if (interval == 0)
> - {
> - vty_out (vty, "Malformed interval string%s", VTY_NEWLINE);
> - return CMD_WARNING;
> - }
> -
> - /* Don't schedule duplicate dumps if the dump command is given
> twice */
> - if (interval == bgp_dump->interval &&
> - type == bgp_dump->type &&
> - path && bgp_dump->filename && !strcmp (path,
> bgp_dump->filename))
> - {
> - return CMD_SUCCESS;
> - }
> -
> - /* Set interval. */
> - bgp_dump->interval = interval;
> - if (bgp_dump->interval_str)
> - free (bgp_dump->interval_str);
> - bgp_dump->interval_str = strdup (interval_str);
> -
> - }
> - else
> - {
> - interval = 0;
> - }
> -
> - /* Create interval thread. */
> - bgp_dump_interval_add (bgp_dump, interval);
> -
> - /* Set type. */
> - bgp_dump->type = type;
> -
> - /* Set file name. */
> - if (bgp_dump->filename)
> - free (bgp_dump->filename);
> - bgp_dump->filename = strdup (path);
> -
> - /* This should be called when interval is expired. */
> - bgp_dump_open_file (bgp_dump);
> -
> - return CMD_SUCCESS;
> -}
> -
> -static int
> bgp_dump_unset (struct vty *vty, struct bgp_dump *bgp_dump)
> {
> /* Set file name. */
> @@ -674,6 +619,69 @@ bgp_dump_unset (struct vty *vty, struct bgp_dump
> *bgp_dump)
> return CMD_SUCCESS;
> }
>
> +static int
> +bgp_dump_set (struct vty *vty, struct bgp_dump *bgp_dump,
> + enum bgp_dump_type type, const char *path,
> + const char *interval_str)
> +{
> + unsigned int interval;
> +
> + /* Don't schedule duplicate dumps if the dump command is given twice */
> + if (bgp_dump->filename && strcmp(path, bgp_dump->filename) == 0
> + && type == bgp_dump->type)
> + {
> + if (interval_str)
> + {
> + if (bgp_dump->interval_str &&
> + strcmp(bgp_dump->interval_str, interval_str) == 0)
> + return CMD_SUCCESS;
> + }
> + else
> + {
> + if (!bgp_dump->interval_str)
> + return CMD_SUCCESS;
> + }
> + }
> +
> + /* Removing previous config */
> + bgp_dump_unset(vty, bgp_dump);
> +
> + if (interval_str)
> + {
> + /* Check interval string. */
> + interval = bgp_dump_parse_time (interval_str);
> + if (interval == 0)
> + {
> + vty_out (vty, "Malformed interval string%s", VTY_NEWLINE);
> + return CMD_WARNING;
> + }
> + /* Setting interval string */
> + bgp_dump->interval_str = strdup (interval_str);
> + }
> + else
> + {
> + interval = 0;
> + }
> +
> + /* Set type. */
> + bgp_dump->type = type;
> +
> + /* Set interval */
> + bgp_dump->interval = interval;
> +
> + /* Set file name. */
> + bgp_dump->filename = strdup (path);
> +
> + /* Create interval thread. */
> + bgp_dump_interval_add (bgp_dump, interval);
> +
> + /* This should be called when interval is expired. */
> + bgp_dump_open_file (bgp_dump);
> +
> + return CMD_SUCCESS;
> +}
> +
> +
> DEFUN (dump_bgp_all,
> dump_bgp_all_cmd,
> "dump bgp all PATH",
>
>
>
>
> On 16 Oct 2015, at 03:36, Paul Jakma <[email protected]> wrote:
>
> On Tue, 13 Oct 2015, Alexis Fasquel wrote:
>
> Hi Paul,
>
> Thanks for the feedback!
>
> So first things first, here’s the RFC link for the Extended MRT Header:
> https://tools.ietf.org/html/rfc6396#section-3 <
> https://tools.ietf.org/html/rfc6396#section-3>. I agree that is not
> perfect however I understand that they must ran out of options here.
>
> And sure, I can definitely make this a runtime config thing. I’m not sure
> what command would like me to add though. My first thought would be to
> change the “dump” command and add an “emrt” parameter:
>
>
> dump bpp updates/routes/all path interval
> dump bpp emrt updates/routes/all path interval
>
>
> This would also allows to dump both with extended Header and “normal”
> Header. However, you might want to have this as a standalone command?
>
>
> I think probably you can it as an optional flag at the end of the existing
> commands? Just add:
>
> [emrt]
>
> at the end, and you can tell if it's there with argc.
>
> Let me know,
>
>
> regards,
> --
> Paul Jakma [email protected] @pjakma Key ID: 64A2FF6A
> Fortune:
> Polly Clarke: My husband. Now there was a man who really was afraid of
> Virginia Woolf.
> Father Ted: Why? Was she... following him or
> something?_______________________________________________
> Quagga-dev mailing list
> [email protected]
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>
>
>
>
> _______________________________________________
> Quagga-dev mailing list
> [email protected]
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
