Hi!
I'm trying to understand the code for corosync. Admittedly it's among the most terrible C code I've seen. I started at one spot, and found some strange coding like this:
if (strcmp(rrp_instance->totem_config->rrp_mode, "active") == 0)
..
if (strcmp(rrp_instance->totem_config->rrp_mode, "passive") == 0)
..
As obviously "rrp_mode" cannot be "active" and "passive" at the same time, why not using an "else if"? Despite of that I wonder why the full string is copied and compared. Shouldn't an enum do as well (Actually I haven't found any enum in the files I had looked at)? (An enum would essentially be the same as using just the first character of "active", "passive" and "none"...
And as there are structures none_algo, passive_algo, and active_algo, each having a ".name" that has the string also (as a reference), why not use a reference to one of these strings to decide what mode the ring has? You could also define one constant instance for each RRP mode string, and then just store the pointer to one of these strings to define the mode. You just do a pointer comparison instead of a string compare...
Unfortunately I didn't understand the complex dependencies of the data structures, so I just make two "style changes" (no functional change, just programming style):
1) simplify the querying of rrp_mode
2) Use a symbolic name for a string constant instead of repeating the string literally wherever it is used. I guessed what the best place for defining these symbolic names might be, using exec/totemrrp.h.
See attached patches.
Regards,
Ulrich
>From 48c67078c74b7ebe22747c8d96fafae8c6ac4ab0 Mon Sep 17 00:00:00 2001
From: Ulrich Windl <Ulrich.Windl@xxxxxxxxxxxxxxxxxxxx>
Date: Wed, 10 Jun 2015 11:20:31 +0200
Subject: [PATCH 2/2] Introduce symbolic names for RRP modes
ChangeLog: Describe these changes.
exec/totemrrp.h: Add #defines for TOTEM_RRP_MODE_NONE,
TOTEM_RRP_MODE_ACTIVE, TOTEM_RRP_MODE_PASSIVE.
exec/totemrrp.c: Replace literal strings with new symbolic constants.
exec/totemconfig.c: Include "totemrrp.h". Replace literal strings with new
symbolic constants.
---
ChangeLog | 17 +++++++++++++++++
exec/totemconfig.c | 7 ++++---
exec/totemrrp.c | 10 +++++-----
exec/totemrrp.h | 4 ++++
4 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 5df871b..b8cd2a2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2015-06-09 Ulrich Windl <Ulrich.Windl@xxxxxxxxxxxxxxxxxxxx>
+
+ * exec/totemconfig.c (totem_config_validate): Replace literal
+ strings with TOTEM_RRP_MODE_NONE, TOTEM_RRP_MODE_ACTIVE and
+ TOTEM_RRP_MODE_PASSIVE, respectively.
+ Include "totemrrp.h".
+
+ * exec/totemrrp.c (none_algo): Use TOTEM_RRP_MODE_NONE instead of
+ literal string.
+ (passive_algo): Use TOTEM_RRP_MODE_PASSIVE instead of literal string.
+ (active_algo): Use TOTEM_RRP_MODE_ACTIVE instead of literal string.
+ (timer_function_test_ring_timeout): Replace literal strings with
+ TOTEM_RRP_MODE_ACTIVE and TOTEM_RRP_MODE_PASSIVE, respectively.
+
+ * exec/totemrrp.h: Add #defines for TOTEM_RRP_MODE_NONE,
+ TOTEM_RRP_MODE_ACTIVE, TOTEM_RRP_MODE_PASSIVE.
+
2014-06-02 Jan Friesse <jfriesse@xxxxxxxxxx>
Move ringid store and load from totem library
diff --git a/exec/totemconfig.c b/exec/totemconfig.c
index 99dd815..4d44eb6 100644
--- a/exec/totemconfig.c
+++ b/exec/totemconfig.c
@@ -64,6 +64,7 @@
#include "util.h"
#include "totemconfig.h"
+#include "totemrrp.h"
#include "tlist.h" /* for HZ */
#define TOKEN_RETRANSMITS_BEFORE_LOSS_CONST 4
@@ -661,9 +662,9 @@ int totem_config_validate (
/*
* RRP values validation
*/
- if (strcmp (totem_config->rrp_mode, "none") &&
- strcmp (totem_config->rrp_mode, "active") &&
- strcmp (totem_config->rrp_mode, "passive")) {
+ if (strcmp (totem_config->rrp_mode, TOTEM_RRP_MODE_NONE) &&
+ strcmp (totem_config->rrp_mode, TOTEM_RRP_MODE_ACTIVE) &&
+ strcmp (totem_config->rrp_mode, TOTEM_RRP_MODE_PASSIVE)) {
snprintf (local_error_reason, sizeof(local_error_reason),
"The RRP mode \"%s\" specified is invalid. It must be none, active, or passive.\n", totem_config->rrp_mode);
goto parse_error;
diff --git a/exec/totemrrp.c b/exec/totemrrp.c
index 6f80fb8..92af9ab 100644
--- a/exec/totemrrp.c
+++ b/exec/totemrrp.c
@@ -524,7 +524,7 @@ struct deliver_fn_context {
};
struct rrp_algo none_algo = {
- .name = "none",
+ .name = TOTEM_RRP_MODE_NONE,
.initialize = NULL,
.mcast_recv = none_mcast_recv,
.mcast_noflush_send = none_mcast_noflush_send,
@@ -543,7 +543,7 @@ struct rrp_algo none_algo = {
};
struct rrp_algo passive_algo = {
- .name = "passive",
+ .name = TOTEM_RRP_MODE_PASSIVE,
.initialize = passive_instance_initialize,
.mcast_recv = passive_mcast_recv,
.mcast_noflush_send = passive_mcast_noflush_send,
@@ -562,7 +562,7 @@ struct rrp_algo passive_algo = {
};
struct rrp_algo active_algo = {
- .name = "active",
+ .name = TOTEM_RRP_MODE_ACTIVE,
.initialize = active_instance_initialize,
.mcast_recv = active_mcast_recv,
.mcast_noflush_send = active_mcast_noflush_send,
@@ -618,9 +618,9 @@ static void timer_function_test_ring_timeout (void *context)
.endian_detector = ENDIAN_LOCAL,
};
- if (strcmp(rrp_instance->totem_config->rrp_mode, "active") == 0)
+ if (strcmp(rrp_instance->totem_config->rrp_mode, TOTEM_RRP_MODE_ACTIVE) == 0)
faulty = ((struct active_instance *)(rrp_instance->rrp_algo_instance))->faulty;
- else if (strcmp(rrp_instance->totem_config->rrp_mode, "passive") == 0)
+ else if (strcmp(rrp_instance->totem_config->rrp_mode, TOTEM_RRP_MODE_PASSIVE) == 0)
faulty = ((struct passive_instance *)(rrp_instance->rrp_algo_instance))->faulty;
else
assert (0);
diff --git a/exec/totemrrp.h b/exec/totemrrp.h
index ca06ee9..ff7bafe 100644
--- a/exec/totemrrp.h
+++ b/exec/totemrrp.h
@@ -43,6 +43,10 @@
#define TOTEMRRP_NOFLUSH 0
#define TOTEMRRP_FLUSH 1
+#define TOTEM_RRP_MODE_NONE "none"
+#define TOTEM_RRP_MODE_ACTIVE "active"
+#define TOTEM_RRP_MODE_PASSIVE "passive"
+
/*
* Totem Network interface - also does encryption/decryption
* depends on poll abstraction, POSIX, IPV4
--
1.7.12.4
>From d042078ceee9c7f9497bc627d8f0be016043e362 Mon Sep 17 00:00:00 2001
From: Ulrich Windl <Ulrich.Windl@xxxxxxxxxxxxxxxxxxxx>
Date: Wed, 13 May 2015 16:00:32 +0200
Subject: [PATCH 1/2] exec/totemrrp.c: Improve checking rrp_mode
exec/totemrrp.c: In timer_function_test_ring_timeout() replace ifs with
if .. else if .. else .. to avoid extra strcmp() if rrp_mode is "active".
Initializing faulty is no longer necessary, also simpifying assert().
---
exec/totemrrp.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/exec/totemrrp.c b/exec/totemrrp.c
index e76d346..6f80fb8 100644
--- a/exec/totemrrp.c
+++ b/exec/totemrrp.c
@@ -611,7 +611,7 @@ static void timer_function_test_ring_timeout (void *context)
{
struct deliver_fn_context *deliver_fn_context = (struct deliver_fn_context *)context;
struct totemrrp_instance *rrp_instance = deliver_fn_context->instance;
- unsigned int *faulty = NULL;
+ unsigned int *faulty;
int iface_no = deliver_fn_context->iface_no;
struct message_header msg = {
.type = MESSAGE_TYPE_RING_TEST_ACTIVE,
@@ -620,10 +620,10 @@ static void timer_function_test_ring_timeout (void *context)
if (strcmp(rrp_instance->totem_config->rrp_mode, "active") == 0)
faulty = ((struct active_instance *)(rrp_instance->rrp_algo_instance))->faulty;
- if (strcmp(rrp_instance->totem_config->rrp_mode, "passive") == 0)
+ else if (strcmp(rrp_instance->totem_config->rrp_mode, "passive") == 0)
faulty = ((struct passive_instance *)(rrp_instance->rrp_algo_instance))->faulty;
-
- assert (faulty != NULL);
+ else
+ assert (0);
if (faulty[iface_no] == 1) {
msg.ring_number = iface_no;
--
1.7.12.4
_______________________________________________
discuss mailing list
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss
