I agree it sucks, but put the declaration at the top of the source file. If you do want to move it, move it first in one patch and then do the rework you want in a second. I'd be ok with that approach.
donald On Thu, Oct 22, 2015 at 8:49 PM, Alexis Fasquel <[email protected]> wrote: > 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]> 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 > > >
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
