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
signature.asc
Description: Message signed with OpenPGP using GPGMail
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
