ipmi: add support for system event log (SEL). #2091
Conversation
Listen for ipmi sensor threshold and discrete events. Send notification in case event is received. Fixed typos. Change-Id: I7613c4656bfec25dbe779b94730ae9e196c3aa9a Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>
Change-Id: I3b2780843cb80bc1c4b2ef72bcbe99eb4cfcf35a Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>
|
Hi! Your patch has |
|
Hi @rpv-tomsk, I understand that deleting this event may affect other registered callbacks which will receive an empty event in this case. I agree to make it optional. What do you think about additional config option like |
Make deletion of event from SEL list configurable to avoid other tools that may be subscribed for SEL events to receive an empty event. Change-Id: Ibd578c8652d9d6df8ee28825f20df356ef46b840 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>
System management interface (SMI) is listening for events from BMC and processes them to user without delays. When SMI receives an event force domain to reread SELs. It will cause event handlers for discrete and threshold sensors to be called. Change-Id: I006d203df1e1fc69d641d1656935b06a38ee1345 Signed-off-by: Korynkevych, RomanX <romanx.korynkevych@intel.com>
|
@rpv-tomsk any additional feedback here? |
|
Hi Maryam! I have no other feedbacks here. Thanks for cooperation! |
There was a problem hiding this comment.
Hi @rkorynkx, thank you very much for your work!
I found two important issues that need to be fixed:
- You cannot assign the return value of
ipmi_event_get_timestamp(event)ton.timeverbatim. You need to use theNS_TO_CDTIME_T()macro. - The meta data that is allocated while handling events is leaked and needs to be freed.
Best regards,
—octo
src/ipmi.c
Outdated
| static int c_ipmi_nofiy_add = 0; | ||
| static int c_ipmi_nofiy_remove = 0; | ||
| static int c_ipmi_nofiy_notpresent = 0; | ||
| static int c_ipmi_notify_add = 0; |
There was a problem hiding this comment.
Nit: these should be of type _Bool.
src/ipmi.c
Outdated
| } | ||
| sensor_name_ptr = sensor_name; | ||
|
|
||
| assert(buf != NULL); |
There was a problem hiding this comment.
Please don't do argument checking with assert(). Instead, add something like this to the beginning of the function.
if ((buf == NULL) || (buf_len == 0)) {
return;
}
src/ipmi.c
Outdated
| c_ipmi_sensor_list_t *list_item; | ||
| c_ipmi_sensor_list_t *list_prev; | ||
|
|
||
| static void sensor_get_name(ipmi_sensor_t *sensor, char *buf, int buf_len) { |
There was a problem hiding this comment.
I find having both, buf and buffer a bit confusing. I suggest to rename buffer to temp and buf to buffer.
src/ipmi.c
Outdated
| return (IPMI_EVENT_NOT_HANDLED); | ||
|
|
||
| sensor_get_name(sensor, buf, sizeof(buf)); | ||
| sstrncpy(n.type_instance, buf, sizeof(n.type_instance)); |
There was a problem hiding this comment.
This seems like an unnecessary copy since you can also do:
sensor_get_name(sensor, n.type_instance, sizeof(n.type_instance));
src/ipmi.c
Outdated
| * the event will receive a NULL for the event. So be ready to handle a NULL | ||
| * event in all your event handlers. A NULL may also be passed to an event | ||
| * handler if the callback was not due to an event. */ | ||
| if (event == NULL) |
There was a problem hiding this comment.
Shouldn't this be the first thing we do in the function? We don't use the return value of ipmi_get_reading_name() if e == NULL.
src/ipmi.c
Outdated
|
|
||
| sstrncpy(n.host, hostname_g, sizeof(n.host)); | ||
| sstrncpy(n.type, ipmi_sensor_get_sensor_type_string(sensor), sizeof(n.type)); | ||
| n.time = ipmi_event_get_timestamp(event); |
There was a problem hiding this comment.
You need to use NS_TO_CDTIME_T() (from "utils_time.h") to convert the IPMI timestamp to collectd's internal time representation.
src/ipmi.c
Outdated
| add_event_common_data(&n, sensor, dir, event); | ||
|
|
||
| plugin_dispatch_notification(&n); | ||
|
|
There was a problem hiding this comment.
The meta data leaks here, please call plugin_notification_meta_free(&n) to free the memory.
src/ipmi.c
Outdated
| add_event_common_data(&n, sensor, dir, event); | ||
|
|
||
| plugin_dispatch_notification(&n); | ||
|
|
There was a problem hiding this comment.
The meta data leaks here, please call plugin_notification_meta_free(&n) to free the memory.
src/ipmi.c
Outdated
| @@ -441,7 +647,7 @@ static void entity_sensor_update_handler( | |||
| */ | |||
| static void domain_entity_update_handler( | |||
| enum ipmi_update_e op, ipmi_domain_t __attribute__((unused)) * domain, | |||
| ipmi_entity_t *entity, void __attribute__((unused)) * user_data) { | |||
| ipmi_entity_t * entity, void __attribute__((unused)) * user_data) { | |||
There was a problem hiding this comment.
This looks like something clang-format should take care of. Please make sure to run it when preparing commits.
src/ipmi.c
Outdated
| c_ipmi_nofiy_notpresent = 1; | ||
| c_ipmi_notify_notpresent = 1; | ||
| } else if (strcasecmp("SELEnabled", key) == 0) { | ||
| if (IS_TRUE(value)) |
There was a problem hiding this comment.
Please use c_ipmi_sel_enabled = IS_TRUE(value); here and elsewhere. If a user ends up with a config like this:
SELEnabled true
SELEnabled false
the second statement should be able to overwrite the previous statement. I'd be very grateful if you could also fix the existing options, thank you!
Signed-off-by: Mytnyk, Volodymyr <volodymyrx.mytnyk@intel.com>
Signed-off-by: Mytnyk, Volodymyr <volodymyrx.mytnyk@intel.com>
|
Hi @octo! Thanks for reviewing the changes and your feedback. I've addressed your comments in last two commits. Also, performed clang-format just to make sure all formatting issues are fixed. I haven't re-based the changes as I don't want to break the whole history. So, if you are ok with the changes please let me know, so I can re-base them. Regards, |
Listen for ipmi sensor threshold and discrete events. Send notification in case event is received. Fix typos.