New plugin: redfish #2926

mmozejkx · 2018-09-14T11:43:46Z

This is a C plugin that collects sensor data using Redfish protocol.
It can get multiple resources from different hardware that support Redfish.

NOTE: Version of this code is an early drop.
Any feedback appreciated.

ChangeLog: Redfish plugin: Collect sensor data from Redfish endpoints.

[edit @kkepka:] added changelog

- Collect data from Redfish interface - Use libredfish C library Change-Id: I95e0ee23ccb11f617d41c9edb0b57c651e5cbdb5 Signed-off-by: Mozejko, MarcinX <marcinx.mozejko@intel.com>

octo

Hi @mmozejkx,

thank you very much for your PR! I've left a bunch of comments inline; don't worry, it's all just coding style. The approach looked good to me, though I don't know Redfish well. Unfortunately I didn't have time to look at the tests, but I'm happy with any tests that we have :)

Best regards,
—octo

octo · 2018-10-05T19:32:33Z

src/redfish.c

+    /* Preparing query pointers list for every service */
+    for (size_t i = 0; i < service->queries_num; i++) {
+      redfish_query_t *ptr;
+      if (c_avl_get(ctx->queries, service->queries[i], (void **)&ptr) != 0)


I'd cast to (void *) here to improve portability, i.e.: (void *)&ptr

octo · 2018-10-05T19:33:36Z

src/redfish.c

+  for (llentry_t *le = llist_head(ctx->services); le != NULL; le = le->next) {
+    redfish_service_t *service = (redfish_service_t *)le->value;
+    /* Ignore redfish version */
+    service->flags |= 0x00000001;


The hex notation and bitwise or'ing indicate a magic constant. Is there a symbolic name you could use instead?

octo · 2018-10-05T19:34:21Z

src/redfish.c

+  return 0;
+
+error:
+  ERROR(PLUGIN_NAME ": Failed to allocate memory for service queries list");


You can get here for other reasons. I'd recomment printing the error message before using goto.

octo · 2018-10-05T19:34:50Z

src/redfish.c

+};
+typedef union redfish_value_u redfish_value_t;
+
+redfish_ctx_t *ctx;


This should be static.

octo · 2018-10-05T19:36:30Z

src/redfish.c

+    goto free_ctx;
+
+  /* Creating placeholder for queries */
+  ctx->queries = c_avl_create((int (*)(const void *, const void *))strcmp);


Again, just cast to (void *) – this cast is hindering readability more than it is helping.

octo · 2018-10-05T20:12:47Z

src/redfish.c

+    redfish_service_t *service = (redfish_service_t *)le->value;
+    for (llentry_t *le = llist_head(service->query_ptrs); le != NULL;
+         le = le->next) {
+      redfish_query_t *query = (redfish_query_t *)le->value;


Optional: unnecessary cast.

octo · 2018-10-05T20:13:01Z

src/redfish.c

+
+static int redfish_read(__attribute__((unused)) user_data_t *ud) {
+  for (llentry_t *le = llist_head(ctx->services); le != NULL; le = le->next) {
+    redfish_service_t *service = (redfish_service_t *)le->value;


Optional: unnecessary cast.

octo · 2018-10-05T20:14:27Z

src/redfish.c

+
+    llist_destroy(service->query_ptrs);
+
+    sfree(service->name);


I've seen this list of sfree statements for various service members before. Maybe put that into a service_destroy() function?

octo · 2018-10-05T20:15:00Z

src/redfish.c

+
+    cleanupServiceEnumerator(service->redfish);
+    for (size_t i = 0; i < service->queries_num; i++)
+      sfree(service->queries[i]);


Optional: Consider using strarray_free().

octo · 2018-10-05T20:15:27Z

src/redfish.c

+  char *key;
+  redfish_query_t *query;
+
+  while (c_avl_iterator_next(i, (void **)&key, (void **)&query) == 0) {


Cast to (void *)

Add unit tests. Change-Id: Iecef07b148ca2ac067cf912bb4a6af2d2190b810 Signed-off-by: Adrian Boczkowski <adrianx.boczkowski@intel.com>

mmozejkx · 2018-10-26T11:15:14Z

@octo Thanks for your feedback. We're working on creating a patch and it should be upstreamed soon.

Regards,
Marcin

Fix segmentation fault Fix memory leaks Change-Id: I8691c292af323145536528f16525206acbfb03f7 Signed-off-by: Mozejko, MarcinX <marcinx.mozejko@intel.com> Signed-off-by: Adrian Boczkowski <adrianx.boczkowski@intel.com>

mkobyli · 2018-11-13T15:16:38Z

Hi @octo,
We updated our PR.
I'll be happy for your review. Please let me know, if PR need any fix.
Regards,
Michal

sunkuranganath · 2019-01-31T14:55:46Z

@mkobyli Could you please resolve the conflicts?

kkepka · 2019-02-18T10:24:56Z

Makefile.am

+redfish_la_LIBADD = $(BUILD_WITH_LIBREDFISH_LIBS) -lredfish
+
+test_plugin_redfish_SOURCES = src/redfish_test.c \
+                              src/daemon/utils_avltree.c \


Please followup on changed location of utils_avltree.

- Collect data from Redfish interface - Use libredfish C library redfish plugin: addressing comments from PR collectd#2926 Add unit tests. Fix race condition. Fix segmentation fault. Fix memory leaks. ChangeLog: Redfish plugin: Collect sensor data using Redfish protocol. Signed-off-by: Marcin Mozejko <marcinx.mozejko@intel.com> Signed-off-by: Adrian Boczkowski <adrianx.boczkowski@intel.com> Signed-off-by: Zoltan Szabo <zoltan.4.szabo@nokia.com> Signed-off-by: Michal Kobylinski <michal.kobylinski@intel.com>

kkepka · 2020-01-14T13:52:06Z

my apologies for "merge ladder", should squash it..

dago

LGTM

New redfish plugin

bca2ce3

- Collect data from Redfish interface - Use libredfish C library Change-Id: I95e0ee23ccb11f617d41c9edb0b57c651e5cbdb5 Signed-off-by: Mozejko, MarcinX <marcinx.mozejko@intel.com>

rpv-tomsk added the New plugin label Sep 24, 2018

rpv-tomsk added this to the 5.9 milestone Sep 24, 2018

octo reviewed Oct 5, 2018

View reviewed changes

redfish plugin: addressing comments from PR collectd#2926

79bb8a0

Add unit tests. Change-Id: Iecef07b148ca2ac067cf912bb4a6af2d2190b810 Signed-off-by: Adrian Boczkowski <adrianx.boczkowski@intel.com>

Redfish plugin: Fix race condition

05302d8

Fix segmentation fault Fix memory leaks Change-Id: I8691c292af323145536528f16525206acbfb03f7 Signed-off-by: Mozejko, MarcinX <marcinx.mozejko@intel.com> Signed-off-by: Adrian Boczkowski <adrianx.boczkowski@intel.com>

mkobyli mentioned this pull request Nov 13, 2018

Redfish plugin [wip] #2890

Closed

Merge branch 'master' into feat_redfish

b9083ec

kkepka reviewed Feb 18, 2019

View reviewed changes

mrunge modified the milestones: 5.9, 5.10 Jul 10, 2019

mmozejkx and others added 2 commits August 23, 2019 12:37

fixed code format

7a8f30a

mrunge modified the milestones: 5.10.0, 5.11.0 Oct 11, 2019

mkobyli and others added 4 commits January 10, 2020 14:18

fixed gather data from sensors

03d78f9

Merge branch 'master' into feat_redfish

863d9ec

Merge branch 'master' into upstream_redfish_v0.2

65645fc

Merge branch 'upstream_redfish_v0.2' into feat_redfish

fa839cc

fix: potential null pointer dereference

13494d4

dago linked an issue Feb 17, 2020 that may be closed by this pull request

Redfish plugin [wip] #2890

Closed

dago approved these changes Feb 28, 2020

View reviewed changes

dago merged commit 1ec0d53 into collectd:master Feb 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New plugin: redfish #2926

New plugin: redfish #2926

mmozejkx commented Sep 14, 2018 •

edited by kkepka

Loading

octo left a comment

octo Oct 5, 2018

octo Oct 5, 2018

octo Oct 5, 2018

octo Oct 5, 2018

octo Oct 5, 2018

octo Oct 5, 2018

octo Oct 5, 2018

octo Oct 5, 2018

octo Oct 5, 2018

octo Oct 5, 2018

mmozejkx commented Oct 26, 2018

mkobyli commented Nov 13, 2018

sunkuranganath commented Jan 31, 2019

kkepka Feb 18, 2019

kkepka commented Jan 14, 2020

dago left a comment

New plugin: redfish #2926

New plugin: redfish #2926

Conversation

mmozejkx commented Sep 14, 2018 • edited by kkepka Loading

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmozejkx commented Oct 26, 2018

mkobyli commented Nov 13, 2018

sunkuranganath commented Jan 31, 2019

Choose a reason for hiding this comment

kkepka commented Jan 14, 2020

dago left a comment

Choose a reason for hiding this comment

mmozejkx commented Sep 14, 2018 •

edited by kkepka

Loading