Return 1 in case of error and fix OpenBSD compilation

[ledeuns]

Returning not 0 when an error occurs is useful when using bgpq3 in a script to generate a lot of filters

[snar]

On Mon, Jun 12, 2017 at 10:03:47AM -0700, Denis Fondras wrote:

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

You can view, comment on, or merge this pull request online at:

https://github.com/snar/bgpq3/pull/33

Commit Summary

• Fix missing header when compiling with OpenBSD6.1

Thanks for noting this problem. Fixed in a bit different way, by the means of checking and conditionally including <sys/select.h> if present. Can I ask you to pull fresh head and check if latest commit fixes this issue ?

• Return 1 in case of error

This code returns 1 not when there is a problem, but when generated object is empty (f.e., prefix-list without prefixes). Can you explain why this situation shall be considered as problematic as "real problems" (like network problems talking radb) ?

File Changes

• M bgpq3.c (8) • M bgpq3_printer.c (7) • M bgpq_expander.c (5)

Patch Links:

• https://github.com/snar/bgpq3/pull/33.patch • https://github.com/snar/bgpq3/pull/33.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.*

[ledeuns]

Compiling on OpenBSD6.1 is still failing because HAVE_SYS_SELECT_H is not defined. Here is a fix :

diff --git a/config.h.in b/config.h.in
index c9d4ed9..4e20cf2 100644
--- a/config.h.in
+++ b/config.h.in
@@ -45,6 +45,9 @@
 /* Define to 1 if you have the <sys/types.h> header file. */
 #undef HAVE_SYS_TYPES_H

+/* Define to 1 if you have the <sys/select.h> header file. */
+#undef HAVE_SYS_SELECT_H
+
 /* Define to 1 if you have the <unistd.h> header file. */
 #undef HAVE_UNISTD_H

About the return code. At our local IXP, we are using bgpq3 to generate authorized prefix-list on the routeservers. There is a script that routinely update prefixlists for every peer and reload openbgpd. When the prefix-list is empty, bgpq3 generates a "deny from AS 0" which translates to "deny from any". If I reload openbgpd with that configuration directive, you can imagine the problem on the routeservers :) Being able to catch that early (ie. when the prefix-list is generated) is useful for my case.

[ledeuns]

Since the commit 810a0595e14dc4a986193ab21453216f8a8e1288, building on OpenBSD6.1 is OK.

[snar]

On Tue, Jun 13, 2017 at 09:50:57AM -0700, Denis Fondras wrote:

Compiling on OpenBSD6.1 is till failing because HAVE_SYS_TYPES_H is not defined. Here is a fix :

Yeah, forgot to run autoheader before commiting :(

About the return code. At our local IXP, we are using bgpq3 to generate authorized prefix-list on the routeservers. There is a script that routinely update prefixlists for every peer and reload openbgpd. When the prefix-list is empty, bgpq3 generates a "deny from AS 0" which translates to "deny from any". If I reload openbgpd with that configuration directive, you can imagine the problem on the routeservers :) Being able to catch that early (ie. when the prefix-list is generated) is useful for my case.

I think that not emitting wrong output is a better idea than catching it :) And 'deny any any' is indeed wrong in this case.

So, new flag -a allows you to specify which asn shall be denied in case when resulting prefix-list is empty:

bgpq3>./bgpq3 -Bl nonex -a 23456 as-nonexistant

generated prefix-list nonex (AS 23456) is empty

deny from AS 23456

If you forget to specify peer-asn with -a, 'deny any any' is not generated anymore (well, it is generated but commented out):

bgpq3>./bgpq3 -Bl nonex as-nonexistant

generated prefix-list nonex (AS 0) is empty

deny from AS 0

PS: Other idea was to generate empty prefix-list in this case (we using this approach with Junpers: empty prefix-list means 'do not accept nothing'), but looks like openbgpd does not allow prefix-lists to be empty :(

[ledeuns]

So, new flag -a allows you to specify which asn shall be denied in case when resulting prefix-list is empty: bgpq3>./bgpq3 -Bl nonex -a 23456 as-nonexistant

Wonderful, thank you !

[snar]

On Wed, Jun 14, 2017 at 05:48:50AM -0700, Denis Fondras wrote:

So, new flag -a allows you to specify which asn shall be denied in case
when resulting prefix-list is empty:
bgpq3>./bgpq3 -Bl nonex -a 23456 as-nonexistant

Wonderful, thank you !

Second thoughs: this patch is somewhat ugly too :(

When user does not specify peer-as and resulting prefix-list is empty, no prefix-filter and no as-filter configured as result, and configuration looks clean enough to be accepted. So peer is now allowed to announce any prefix he wants.

Proposed quick-and-dirty fix: when user does not specifies peer-as, and resulting prefix-list is empty, empty prefix-list is generated:

bgpq3>./bgpq3 -Bl nonex as-nonexistant

generated prefix-list nonex (AS 0) is empty

use -a to generate "deny from ASN " instead of this list

nonex="prefix { \ }"

that will prevent openbgpd from accepting such configuration.

Long-term: try to fix OpenBGPD, so -a will become unnecessary :)