Steve,
thanks for porting the patch.
i'm happy to make the suggested changes. I should be able to use git
and work against the master. Will post the updated patch as soon
as i have all the corrections done.
Anton.
On Oct 21, 2011, at 2:53 PM, Steven Dake wrote:
> 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
