OpenBMP support #73
OpenBMP support #73
Conversation
|
Forgot to exclude the .clang-format file in my commits, please ignore the file. |
lib/openbmp/parsebgp_openbmp.c
Outdated
| if (memcmp(buf, "OBMP", 4) != 0) { | ||
| // it's not a known OpenBMP header, assume that it is raw BMP | ||
| *buf_len = 0; | ||
| printf("it's not a known OpenBMP header, assume that it is raw BMP\n"); |
There was a problem hiding this comment.
i don't think you just want a printf here.
this code was originally supposed to allow the caller to handle either openbmp-formatted data or raw BMP.
i'm not sure how to support that use case in the libparsebgp model. we could just try parsing the BMP content at this stage, but then we'd need a flag in the openbmp message structure that indicates that only the bmp field is valid. i guess that could work.
lib/openbmp/parsebgp_openbmp.c
Outdated
| } | ||
|
|
||
| // is this an OpenBMP ASCII header (either "text" or "legacy-text")? | ||
| if (*buf == 'V') { |
There was a problem hiding this comment.
let's not support the v1 openbmp headers.
lib/openbmp/parsebgp_openbmp.c
Outdated
| // we want at least a few bytes to do header checks | ||
| if (len < 4) { | ||
| *buf_len = 0; | ||
| printf("we want at least a few bytes to do header checks\n"); |
There was a problem hiding this comment.
please don't use printf's. there are a bunch of utility macros for handling these kinds of parsing failures. in this case you probably just want to return PARSEBGP_PARTIAL_MSG without any kind of logging since this will be a normal failure in the case that the caller hasn't buffered a full message.
lib/openbmp/parsebgp_openbmp.c
Outdated
| if (len < 4) { | ||
| *buf_len = 0; | ||
| printf("we want at least a few bytes to do header checks\n"); | ||
| return 0; |
There was a problem hiding this comment.
return codes should be one of the parsebgp_error_t values
lib/openbmp/parsebgp_openbmp.c
Outdated
| if (msg->ver_maj != 1 || msg->ver_min != 7) { | ||
| printf("Unrecognized OpenBMP header version (%" PRIu8 ".%" PRIu8 ")", | ||
| msg->ver_maj, msg->ver_min); | ||
| return 0; |
There was a problem hiding this comment.
i think you should be returning an error here, no?
this is probably an instance where you want to use one of the INVALID message macros
also removed printf statements.
|
@alistairking I have made the corresponding changes for all comments above. Please review again :) |
Hello,
I added OpenBMP parsing feature to libparsebgp, and it parses messages from raw_bmp topic.
The parsing procedures are copied from libbgpstream.