Anton,
I forward ported your patch to master and posted it to the mailing list
via git send-email (we develop first in master, then backport to avoid
regressions).
Really nice work! There are a few issues that need addressing. If you
are unable to do the updates (against master), please let us know and
someone from the community can pick up the patch.
Don't worry about backporting to a stable version - branch maintainer
will take care of that work.
Patch review below
On 10/21/2011 02:38 PM, Steven Dake wrote:
> From: Anton Jouline <anton.jouline@xxxxxxxxxxxxxxxxxx>
>
> We are using a new db object to keep track of dynamic members, called
> "totem.interface.dynamic". You create and delete child objects of that object,
> using corosync-objctl utility:
>
> to add new member:
> corosync-objctl -c totem.interface.dynamic.10-211-55-12
>
> to delete an existing member:
> corosync-objctl -d totem.interface.dynamic.10-211-55-12
>
> Then the logic in main.c reacts to those events accordingly, by adding a member
> to the member list, or removing an existing one from it. (The object names are
> basically IP addresses with dots replaced by dashes, because the dot is used as
> separator in object hierarchy.)
>
> The patch was successfully tested with Corosync versions 1.4.1 and 1.4.2.
>
> Signed-off-by: Steven Dake <sdake@xxxxxxxxxx>
> ---
> exec/main.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> exec/totempg.c | 14 +++++++
> exec/totemudpu.c | 53 +++++++++++++++++++++++++++
> 3 files changed, 174 insertions(+), 0 deletions(-)
>
> diff --git a/exec/main.c b/exec/main.c
> index aacdc2f..3f40efc 100644
> --- a/exec/main.c
> +++ b/exec/main.c
> @@ -680,6 +680,112 @@ static void corosync_totem_stats_updater (void *data)
> &corosync_stats_timer_handle);
> }
>
> +
> +static void totem_dynamic_create_notify_fn(hdb_handle_t parent_object_handle,
> + hdb_handle_t object_handle,
> + const void *name_pt, size_t name_len,
> + void *priv_data_pt)
> +{
> + struct totem_ip_address member;
> + int ring_no;
> + int numbers[4];
> + int i;
> + char *object_name;
> +
> + object_name = strndup(name_pt, name_len);
should avoid memory allocation if possible. A better choice here would
be a variable on the stack.
> + log_printf(LOGSYS_LEVEL_DEBUG,
> + "create_object: %s\n", object_name);
> +
> + memset(numbers, 0, sizeof numbers);
> + if (sscanf(object_name, "%d-%d-%d-%d",
> + &numbers[0], &numbers[1], &numbers[2], &numbers[3]) == 4) {
sscanf is pretty evil... We provide a parser helper in totemip.h called
totemip_parse.
I saw in the patch message you mentioned the dots were parsed as
separate objects. This should be fixed if this is the case.
> + /*
> + * prepare totem_ip_address struct
> + */
> + ring_no = 0;
> + member.family = AF_INET;
> + for (i=0;i<4;i++) {
> + member.addr[i] = numbers[i];
> + }
> +
> + /*
> + * add new member
> + */
> + totempg_member_add (&member, ring_no);
> + }
> +
> + free(object_name);
no alloc, no free. totemip_parse is non-destructive.
> +}
> +
> +static void totem_dynamic_destroy_notify_fn(hdb_handle_t parent_object_handle,
> + const void *name_pt, size_t name_len,
> + void *priv_data_pt)
> +{
> + struct totem_ip_address member;
> + int ring_no;
> + int numbers[4];
> + int i;
> + char *object_name;
> +
> + object_name = strndup(name_pt, name_len);
> + log_printf(LOGSYS_LEVEL_DEBUG,
> + "destroy_object: %s\n", object_name);
same comments apply to destroy
> +
> + memset(numbers, 0, sizeof numbers);
> + if (sscanf(object_name, "%d-%d-%d-%d",
> + &numbers[0], &numbers[1], &numbers[2], &numbers[3]) == 4) {
> + /* prepare totem_ip_address struct */
> + ring_no = 0;
> + member.family = AF_INET;
> + for (i=0;i<4;i++) {
> + member.addr[i] = numbers[i];
> + }
> +
> + /*
> + * add new member
> + */
> + totempg_member_remove(&member, ring_no);
> + }
> +
> + free(object_name);
> +
> +}
> +
> +static void corosync_totem_dynamic_init (void)
> +{
> + hdb_handle_t object_find_handle;
> + hdb_handle_t object_totem_handle;
> + hdb_handle_t object_interface_handle;
> +
> + if (objdb->object_find_create(OBJECT_PARENT_HANDLE,
> + "totem", strlen("totem"), &object_find_handle) != 0) {
> + log_printf(LOGSYS_LEVEL_NOTICE,
> + "corosync_totem_dynamic_init:: FAILED to find totem!\n");
> + return;
> + }
> + if (objdb->object_find_next (object_find_handle,
> + &object_totem_handle) != 0) {
> + return;
> + }
> +
> + if (objdb->object_find_create(object_totem_handle,
> + "interface", strlen("interface"), &object_find_handle) != 0) {
> + log_printf(LOGSYS_LEVEL_NOTICE,
> + "corosync_totem_dynamic_init:: FAILED to find totem.interface!\n");
The function name is logged as part of the logging infrastructure
already - no need to log it again. This should be LOGSYS_LEVEL_ERROR as
well.
> + return;
> + }
> + if (objdb->object_find_next (object_find_handle,
> + &object_interface_handle) != 0) {
> + return;
> + }
> +
> + objdb->object_track_start (object_interface_handle,
> + OBJECT_TRACK_DEPTH_RECURSIVE,
> + NULL,
> + totem_dynamic_create_notify_fn,
> + totem_dynamic_destroy_notify_fn,
> + NULL, NULL);
> +}
> static void corosync_totem_stats_init (void)
> {
> totempg_stats_t * stats;
> @@ -1128,6 +1234,7 @@ static void main_service_ready (void)
> cs_ipcs_init();
> corosync_totem_stats_init ();
> corosync_fplay_control_init ();
> + corosync_totem_dynamic_init ();
> if (minimum_sync_mode == CS_SYNC_V2) {
> log_printf (LOGSYS_LEVEL_NOTICE, "Compatibility mode set to none. Using V2 of the synchronization engine.\n");
> sync_v2_init (
> diff --git a/exec/totempg.c b/exec/totempg.c
> index 9e88104..99a80e9 100644
> --- a/exec/totempg.c
> +++ b/exec/totempg.c
> @@ -1444,6 +1444,20 @@ void totempg_queue_level_register_callback (totem_queue_level_changed_fn fn)
> totem_queue_level_changed = fn;
> }
>
> + extern int totempg_member_add (
> + const struct totem_ip_address *member,
> + int ring_no)
> +{
> + return totemmrp_member_add (member, ring_no);
> +}
> +
> + extern int totempg_member_remove (
> + const struct totem_ip_address *member,
> + int ring_no)
> +{
> + return totemmrp_member_remove (member, ring_no);
> +}
> +
> extern int totempg_member_add (
> const struct totem_ip_address *member,
> int ring_no);
> diff --git a/exec/totemudpu.c b/exec/totemudpu.c
> index 21e57c7..86e84e8 100644
> --- a/exec/totemudpu.c
> +++ b/exec/totemudpu.c
> @@ -1677,6 +1677,9 @@ int totemudpu_member_add (
> if (new_member == NULL) {
> return (-1);
> }
> + log_printf(LOGSYS_LEVEL_NOTICE, "totemudpu_member_add:: adding new member {%d.%d.%d.%d}\n",
> + member->addr[0], member->addr[1],
> + member->addr[2], member->addr[3]);
totemip_print() should be used here.
> list_init (&new_member->list);
> list_add_tail (&new_member->list, &instance->member_list);
> memcpy (&new_member->member, member, sizeof (struct totem_ip_address));
> @@ -1708,12 +1711,62 @@ int totemudpu_member_add (
> return (0);
> }
>
> +int _same_ip(const struct totem_ip_address *a, const struct totem_ip_address *b);
> +
> +int _same_ip(const struct totem_ip_address *a, const struct totem_ip_address *b)
> +{
> + return a->addr[0] == b->addr[0]
> + && a->addr[1] == b->addr[1]
> + && a->addr[2] == b->addr[2]
> + && a->addr[3] == b->addr[3];
> +
> +}
> +
We already have an equal comparitor function called totemip_equal().
> int totemudpu_member_remove (
> void *udpu_context,
> const struct totem_ip_address *token_target)
> {
> + int found = 0;
> + struct list_head *list;
> + struct totemudpu_member *member;
> +
> struct totemudpu_instance *instance = (struct totemudpu_instance *)udpu_context;
>
> + /*
> + * Find the member to remove and close its socket
> + */
> + for (list = instance->member_list.next;
> + list != &instance->member_list;
> + list = list->next) {
> +
> + member = list_entry (list,
> + struct totemudpu_member,
> + list);
> +
> + if (_same_ip(token_target, &member->member)) {
> + log_printf(LOGSYS_LEVEL_NOTICE, "totemudpu_member_remove:: removing member {%d.%d.%d.%d}\n",
> + member->member.addr[0], member->member.addr[1],
> + member->member.addr[2], member->member.addr[3]);
> +
totemip_print should be used here
> + if (member->fd > 0) {
> + log_printf(LOGSYS_LEVEL_NOTICE, "Closing socket to: {%d.%d.%d.%d}\n",
> + member->member.addr[0], member->member.addr[1],
> + member->member.addr[2], member->member.addr[3]);
> + close (member->fd);
> + cs_poll_dispatch_delete (instance->totemudpu_poll_handle,
> + member->fd);
> + }
> + found = 1;
> + break;
> + }
> + }
> +
> + /*
> + * Delete the member from the list
> + */
> + if (found)
> + list_del(list);
> +
{ } should be used around all conditionals ie:
if (found) {
list_del (list)
}
> instance = NULL;
> return (0);
> }
_______________________________________________
discuss mailing list
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss
