Hello Donald, I understand that moving the function is not optimal to see the diff, however, I did the move it since the new implementation of `bgp_dump_set` is actually calling the `bgp_dump_unset` function. Which means that `bgp_dump_unset` must be defined before the call. Since both `bgp_dump_unset` and `bgp_dump_set` are not declared either on the header file or on the top of the source file, it made sense to just just changed the order and move the `bgp_dump_unset` before `bgp_dump_set`.
But of course, I can definitely create declaration on the top of the source file if you want me to. Alexis > On 21 Oct 2015, at 01:01, Donald Sharp <[email protected]> wrote: > > Why the function move in the patch? It makes it very hard for me to compare > the before and after. What are we gaining by moving the function to later in > the file? > > donald > > On Tue, Oct 20, 2015 at 5:48 PM, Alexis Fasquel <[email protected] > <mailto:[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] > > <mailto:[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> > >> <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] <mailto:[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] <mailto:[email protected]> > > https://lists.quagga.net/mailman/listinfo/quagga-dev > > <https://lists.quagga.net/mailman/listinfo/quagga-dev> > > > _______________________________________________ > Quagga-dev mailing list > [email protected] <mailto:[email protected]> > https://lists.quagga.net/mailman/listinfo/quagga-dev > <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
