Fixed the issue with plugin - FOR-5318 #4215
Conversation
|
Hi @rmahboubian, thank you so much for your contribution! Unfortunately I don't have access to https://bigleafnetworks.atlassian.net/jira/ – can you add to the PR description what the problem is this is solving? The PR description talks about the Python plugin, but the bulk of changes appear to be in the Ping plugin, is that intentional? Best regards |
There was a problem hiding this comment.
Hi @rmahboubian, thank you for your PR! Overall this looks good, just some things we need to iron out ;)
Best regards
—octo
| if (src_host == NULL) { | ||
| ERROR("ping plugin: malloc failed to alloc src_host in submit"); | ||
| } else { |
There was a problem hiding this comment.
Please add a return statement here so the rest of the function does not need to be indented in an else block.
https://google.github.io/styleguide/go/decisions.html#indent-error-flow
| sstrncpy(src_host, source, strlen(source) + 1); | ||
| strcat(src_host, "_"); | ||
| strcat(src_host, host); |
There was a problem hiding this comment.
Please use utils/strbuf/strbuf.h here:
strbuf_t buf = STRBUF_CREATE;
strbuf_printf(&buf, "%s_%s", src_host, host);
/* use buf.ptr, which contains the string */
STRBUF_DESTROY(buf);| sstrncpy(vl.type, type, sizeof(vl.type)); | ||
|
|
||
| plugin_dispatch_values(&vl); | ||
| char *src_host = (char *)malloc(strlen(source) + strlen(host) + 2); |
There was a problem hiding this comment.
src_host is only written to, but never read. Are you missing a
sstrncpy(vl.host, src_host, sizeof(vl.host));in this function?
| hostlist_t *hl; | ||
| char *host = NULL; | ||
| hl = malloc(sizeof(*hl)); |
There was a problem hiding this comment.
Please use calloc instead of malloc so that the buffer is zeroed.
| hostlist_t *hl; | |
| char *host = NULL; | |
| hl = malloc(sizeof(*hl)); | |
| hostlist_t *hl = calloc(1, sizeof(*hl)); | |
| /* declare `host` just before calling `cf_util_get_string` */ |
| sourcelist_t *sl; | ||
| sl = calloc(1, sizeof(*sl)); |
There was a problem hiding this comment.
| sourcelist_t *sl; | |
| sl = calloc(1, sizeof(*sl)); | |
| sourcelist_t *sl = calloc(1, sizeof(*sl)); |
| sourcelist_t *sl; | ||
| if (sourcelist_head == NULL) { | ||
| NOTICE("ping plugin: No sources have been configured."); | ||
| return -1; | ||
| } | ||
| sl = sourcelist_head; |
There was a problem hiding this comment.
| sourcelist_t *sl; | |
| if (sourcelist_head == NULL) { | |
| NOTICE("ping plugin: No sources have been configured."); | |
| return -1; | |
| } | |
| sl = sourcelist_head; | |
| sourcelist_t *sl = sourcelist_head; | |
| if (sl == NULL) { | |
| NOTICE("ping plugin: No sources have been configured."); | |
| return -1; | |
| } |
| status = pthread_join(sl->ping_thread_id, /* return = */ NULL); | ||
| if (status != 0) { | ||
| ERROR("ping plugin: Stopping thread failed."); | ||
| status = -1; |
There was a problem hiding this comment.
This may not work as intended: in the next iteration, status will be reset to the return value of pthread_join, which may be successful. So this function may return zero even though one of the joins failed.
I suggest to introduce a new variable, int last_err = 0; before the for loop and add last_err = status; to the error handling inside the loop.
| if (status != 0) { | ||
| ping_thread_loop = 0; | ||
| ERROR("ping plugin: Starting thread failed."); | ||
| if (ping_thread_loop == 0) { | ||
| pthread_mutex_unlock(&ping_lock); | ||
| return -1; |
There was a problem hiding this comment.
Sorry for commenting on pre-existing code, but this really should return zero.
| return -1; | |
| return 0; |
| if (sl->source != NULL && !sl->is_device) | ||
| if (ping_setopt(pingobj, PING_OPT_SOURCE, (void *)sl->source) != 0) | ||
| ERROR("ping plugin: Failed to set source address: %s", | ||
| ping_get_error(pingobj)); | ||
|
|
||
| #ifdef HAVE_OPING_1_3 | ||
| if (ping_device != NULL) | ||
| if (ping_setopt(pingobj, PING_OPT_DEVICE, (void *)ping_device) != 0) | ||
| if (sl->source != NULL && sl->is_device) | ||
| if (ping_setopt(pingobj, PING_OPT_DEVICE, (void *)sl->source) != 0) | ||
| ERROR("ping plugin: Failed to set device: %s", ping_get_error(pingobj)); | ||
| #endif |
There was a problem hiding this comment.
It was previously possible to set source address and device. By using sourcelist_t.source for both, we lose this functionality.
I suggest that we have two char * fields in sourcelist_t, src_addr and device, and that we remove the is_device boolean. WDYT?
octo
added
the
Waiting for response - 1st time
What does this Change do?
Fixes the charts under performance charts.
Jira ticket number?
https://bigleafnetworks.atlassian.net/jira/software/c/projects/FOR/issues/FOR-5318
How should these changes be verified?
Checklist before approval and merge