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]> 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
