On 5/23/2012 2:55 PM, Steven Dake wrote:
> Reviewed-by: Steven Dake <sdake@xxxxxxxxxx>
Honza also suggested to rename the file mainconfig.c to logconfig.c (and
same for .h) since it does only that effectively.
>
> Perhaps there is a more elegant way?
I am open to any suggestion really. The problem is icmap vs cmap clearly
and we can´t easily macro them all out to keep the code change to the
minimum, but i want to avoid code duplication at all costs.
>
> On 05/23/2012 03:15 AM, Fabio M. Di Nitto wrote:
>> From: "Fabio M. Di Nitto" <fdinitto@xxxxxxxxxx>
>>
>> corosync logging configuration logic is rather complex and in order
>> to make it simpler to reuse (at least within corosync/ tree)
>> we need to be able to use both icmap and cmap.
>>
>> the patch might seem controversial, but it reduces heaps of code around
>> from qdevices (coming next).
>>
>> It might be useful to consider moving this to a common shared library
>> but there aren't enough users yet and a shared lib would force
>> corosync to link with cmap (that we do not want at all costs)
>>
>
> your correct corosync should not link with any ipc libs, super circular
> dependency of the year :)
Well not really no. Linking as indirect dependency is not the same as
using but we agree that it is not what we want :)
Fabio
>
> Regards
> -steve
>> Signed-off-by: Fabio M. Di Nitto <fdinitto@xxxxxxxxxx>
>> ---
>> exec/Makefile.am | 2 +-
>> exec/mainconfig.c | 133 ++++++++++++++++++++++++++++++++++++++--------------
>> exec/mainconfig.h | 8 +++
>> 3 files changed, 106 insertions(+), 37 deletions(-)
>>
>> diff --git a/exec/Makefile.am b/exec/Makefile.am
>> index 9075bcd..338cd46 100644
>> --- a/exec/Makefile.am
>> +++ b/exec/Makefile.am
>> @@ -31,7 +31,7 @@
>>
>> MAINTAINERCLEANFILES = Makefile.in
>>
>> -AM_CFLAGS = -fPIC
>> +AM_CFLAGS = -fPIC -DMAINCONFIG_USE_ICMAP=1
>>
>> INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include $(nss_CFLAGS) $(rdmacm_CFLAGS) $(ibverbs_CFLAGS)
>>
>> diff --git a/exec/mainconfig.c b/exec/mainconfig.c
>> index bf860d0..9ebd9c3 100644
>> --- a/exec/mainconfig.c
>> +++ b/exec/mainconfig.c
>> @@ -34,23 +34,21 @@
>> * THE POSSIBILITY OF SUCH DAMAGE.
>> */
>>
>> -#include <config.h>
>> -
>> -#include <stdio.h>
>> -#include <string.h>
>> -#include <stdlib.h>
>> -#include <errno.h>
>> -#include <unistd.h>
>> -#include <sys/socket.h>
>> -#include <netinet/in.h>
>> -#include <arpa/inet.h>
>> -#include <limits.h>
>> -
>> -#include <corosync/corotypes.h>
>> -#include <corosync/list.h>
>> +#include "config.h"
>> +
>> #include <corosync/totem/totem.h>
>> #include <corosync/logsys.h>
>> +#ifdef MAINCONFIG_USE_ICMAP
>> #include <corosync/icmap.h>
>> +#define MAP_KEYNAME_MAXLEN ICMAP_KEYNAME_MAXLEN
>> +#define map_get_string(key_name, value) icmap_get_string(key_name, value)
>> +#else
>> +#include <corosync/cmap.h>
>> +static cmap_handle_t cmap_handle;
>> +static const char *main_logfile;
>> +#define MAP_KEYNAME_MAXLEN CMAP_KEYNAME_MAXLEN
>> +#define map_get_string(key_name, value) cmap_get_string(cmap_handle, key_name, value)
>> +#endif
>>
>> #include "util.h"
>> #include "mainconfig.h"
>> @@ -140,7 +138,7 @@ static int corosync_main_config_format_set (
>> char *value = NULL;
>> int err = 0;
>>
>> - if (icmap_get_string("logging.fileline", &value) == CS_OK) {
>> + if (map_get_string("logging.fileline", &value) == CS_OK) {
>> if (strcmp (value, "on") == 0) {
>> if (!insert_into_buffer(new_format_buffer,
>> sizeof(new_format_buffer),
>> @@ -168,7 +166,7 @@ static int corosync_main_config_format_set (
>> goto parse_error;
>> }
>>
>> - if (icmap_get_string("logging.function_name", &value) == CS_OK) {
>> + if (map_get_string("logging.function_name", &value) == CS_OK) {
>> if (strcmp (value, "on") == 0) {
>> if (!insert_into_buffer(new_format_buffer,
>> sizeof(new_format_buffer),
>> @@ -196,7 +194,7 @@ static int corosync_main_config_format_set (
>> goto parse_error;
>> }
>>
>> - if (icmap_get_string("logging.timestamp", &value) == CS_OK) {
>> + if (map_get_string("logging.timestamp", &value) == CS_OK) {
>> if (strcmp (value, "on") == 0) {
>> if(!insert_into_buffer(new_format_buffer,
>> sizeof(new_format_buffer),
>> @@ -239,10 +237,10 @@ static int corosync_main_config_log_destination_set (
>> static char formatted_error_reason[128];
>> char *value = NULL;
>> unsigned int mode;
>> - char key_name[ICMAP_KEYNAME_MAXLEN];
>> + char key_name[MAP_KEYNAME_MAXLEN];
>>
>> - snprintf(key_name, ICMAP_KEYNAME_MAXLEN, "%s.%s", path, key);
>> - if (icmap_get_string(key_name, &value) == CS_OK) {
>> + snprintf(key_name, MAP_KEYNAME_MAXLEN, "%s.%s", path, key);
>> + if (map_get_string(key_name, &value) == CS_OK) {
>> if (deprecated) {
>> log_printf(LOGSYS_LEVEL_WARNING,
>> "Warning: the %s config paramater has been obsoleted."
>> @@ -288,7 +286,7 @@ static int corosync_main_config_set (
>> const char *error_reason = error_string_response;
>> char *value = NULL;
>> int mode;
>> - char key_name[ICMAP_KEYNAME_MAXLEN];
>> + char key_name[MAP_KEYNAME_MAXLEN];
>>
>> /*
>> * this bit abuses the internal logsys exported API
>> @@ -320,8 +318,8 @@ static int corosync_main_config_set (
>> LOGSYS_MODE_OUTPUT_SYSLOG, 0, NULL) != 0)
>> goto parse_error;
>>
>> - snprintf(key_name, ICMAP_KEYNAME_MAXLEN, "%s.%s", path, "syslog_facility");
>> - if (icmap_get_string(key_name, &value) == CS_OK) {
>> + snprintf(key_name, MAP_KEYNAME_MAXLEN, "%s.%s", path, "syslog_facility");
>> + if (map_get_string(key_name, &value) == CS_OK) {
>> int syslog_facility;
>>
>> syslog_facility = qb_log_facility2int(value);
>> @@ -338,8 +336,8 @@ static int corosync_main_config_set (
>> free(value);
>> }
>>
>> - snprintf(key_name, ICMAP_KEYNAME_MAXLEN, "%s.%s", path, "syslog_level");
>> - if (icmap_get_string(key_name, &value) == CS_OK) {
>> + snprintf(key_name, MAP_KEYNAME_MAXLEN, "%s.%s", path, "syslog_level");
>> + if (map_get_string(key_name, &value) == CS_OK) {
>> int syslog_priority;
>>
>> log_printf(LOGSYS_LEVEL_WARNING,
>> @@ -359,8 +357,8 @@ static int corosync_main_config_set (
>> free(value);
>> }
>>
>> - snprintf(key_name, ICMAP_KEYNAME_MAXLEN, "%s.%s", path, "syslog_priority");
>> - if (icmap_get_string(key_name, &value) == CS_OK) {
>> + snprintf(key_name, MAP_KEYNAME_MAXLEN, "%s.%s", path, "syslog_priority");
>> + if (map_get_string(key_name, &value) == CS_OK) {
>> int syslog_priority;
>>
>> syslog_priority = logsys_priority_id_get(value);
>> @@ -376,13 +374,21 @@ static int corosync_main_config_set (
>> free(value);
>> }
>>
>> - snprintf(key_name, ICMAP_KEYNAME_MAXLEN, "%s.%s", path, "logfile");
>> - if (icmap_get_string(key_name, &value) == CS_OK) {
>> +#ifdef MAINCONFIG_USE_ICMAP
>> + snprintf(key_name, MAP_KEYNAME_MAXLEN, "%s.%s", path, "logfile");
>> + if (map_get_string(key_name, &value) == CS_OK) {
>> if (logsys_config_file_set (subsys, &error_reason, value) < 0) {
>> goto parse_error;
>> }
>> free(value);
>> }
>> +#else
>> + if (!subsys) {
>> + if (logsys_config_file_set (subsys, &error_reason, main_logfile) < 0) {
>> + goto parse_error;
>> + }
>> + }
>> +#endif
>>
>> if (corosync_main_config_log_destination_set (path, "to_file", subsys, &error_reason,
>> LOGSYS_MODE_OUTPUT_FILE, 1, "to_logfile") != 0)
>> @@ -392,8 +398,8 @@ static int corosync_main_config_set (
>> LOGSYS_MODE_OUTPUT_FILE, 0, NULL) != 0)
>> goto parse_error;
>>
>> - snprintf(key_name, ICMAP_KEYNAME_MAXLEN, "%s.%s", path, "logfile_priority");
>> - if (icmap_get_string(key_name, &value) == CS_OK) {
>> + snprintf(key_name, MAP_KEYNAME_MAXLEN, "%s.%s", path, "logfile_priority");
>> + if (map_get_string(key_name, &value) == CS_OK) {
>> int logfile_priority;
>>
>> logfile_priority = logsys_priority_id_get(value);
>> @@ -409,8 +415,8 @@ static int corosync_main_config_set (
>> free(value);
>> }
>>
>> - snprintf(key_name, ICMAP_KEYNAME_MAXLEN, "%s.%s", path, "debug");
>> - if (icmap_get_string(key_name, &value) == CS_OK) {
>> + snprintf(key_name, MAP_KEYNAME_MAXLEN, "%s.%s", path, "debug");
>> + if (map_get_string(key_name, &value) == CS_OK) {
>> if (strcmp (value, "on") == 0) {
>> if (logsys_config_debug_set (subsys, 1) < 0) {
>> error_reason = "unable to set debug on";
>> @@ -441,10 +447,15 @@ static int corosync_main_config_read_logging (
>> const char **error_string)
>> {
>> const char *error_reason;
>> +#ifdef MAINCONFIG_USE_ICMAP
>> icmap_iter_t iter;
>> const char *key_name;
>> - char key_subsys[ICMAP_KEYNAME_MAXLEN];
>> - char key_item[ICMAP_KEYNAME_MAXLEN];
>> +#else
>> + cmap_iter_handle_t iter;
>> + char key_name[CMAP_KEYNAME_MAXLEN];
>> +#endif
>> + char key_subsys[MAP_KEYNAME_MAXLEN];
>> + char key_item[MAP_KEYNAME_MAXLEN];
>> int res;
>>
>> /* format set is supported only for toplevel */
>> @@ -460,8 +471,13 @@ static int corosync_main_config_read_logging (
>> * we will need 2 of these to compensate for new logging
>> * config format
>> */
>> +#ifdef MAINCONFIG_USE_ICMAP
>> iter = icmap_iter_init("logging.logger_subsys.");
>> while ((key_name = icmap_iter_next(iter, NULL, NULL)) != NULL) {
>> +#else
>> + cmap_iter_init(cmap_handle, "logging.logger_subsys.", &iter);
>> + while ((cmap_iter_next(cmap_handle, iter, key_name, NULL, NULL)) == CS_OK) {
>> +#endif
>> res = sscanf(key_name, "logging.logger_subsys.%[^.].%s", key_subsys, key_item);
>>
>> if (res != 2) {
>> @@ -472,13 +488,17 @@ static int corosync_main_config_read_logging (
>> continue ;
>> }
>>
>> - snprintf(key_item, ICMAP_KEYNAME_MAXLEN, "logging.logger_subsys.%s", key_subsys);
>> + snprintf(key_item, MAP_KEYNAME_MAXLEN, "logging.logger_subsys.%s", key_subsys);
>>
>> if (corosync_main_config_set(key_item, key_subsys, &error_reason) < 0) {
>> goto parse_error;
>> }
>> }
>> +#ifdef MAINCONFIG_USE_ICMAP
>> icmap_iter_finalize(iter);
>> +#else
>> + cmap_iter_finalize(cmap_handle, iter);
>> +#endif
>>
>> logsys_config_apply();
>> return 0;
>> @@ -489,12 +509,23 @@ parse_error:
>> return (-1);
>> }
>>
>> +#ifdef MAINCONFIG_USE_ICMAP
>> static void main_logging_notify(
>> int32_t event,
>> const char *key_name,
>> struct icmap_notify_value new_val,
>> struct icmap_notify_value old_val,
>> void *user_data)
>> +#else
>> +static void main_logging_notify(
>> + cmap_handle_t cmap_handle_unused,
>> + cmap_handle_t cmap_track_handle_unused,
>> + int32_t event,
>> + const char *key_name,
>> + struct cmap_notify_value new_val,
>> + struct cmap_notify_value old_val,
>> + void *user_data)
>> +#endif
>> {
>> const char *error_string;
>>
>> @@ -507,6 +538,7 @@ static void main_logging_notify(
>> corosync_main_config_read_logging(&error_string);
>> }
>>
>> +#ifdef MAINCONFIG_USE_ICMAP
>> static void add_logsys_config_notification(void)
>> {
>> icmap_track_t icmap_track = NULL;
>> @@ -517,12 +549,41 @@ static void add_logsys_config_notification(void)
>> NULL,
>> &icmap_track);
>> }
>> +#else
>> +static void add_logsys_config_notification(void)
>> +{
>> + cmap_track_handle_t cmap_track;
>> +
>> + cmap_track_add(cmap_handle, "logging.",
>> + CMAP_TRACK_ADD | CMAP_TRACK_DELETE | CMAP_TRACK_MODIFY | CMAP_TRACK_PREFIX,
>> + main_logging_notify,
>> + NULL,
>> + &cmap_track);
>> +}
>> +#endif
>>
>> int corosync_main_config_read (
>> +#ifndef MAINCONFIG_USE_ICMAP
>> + cmap_handle_t cmap_h,
>> + const char *default_logfile,
>> +#endif
>> const char **error_string)
>> {
>> const char *error_reason = error_string_response;
>>
>> +#ifndef MAINCONFIG_USE_ICMAP
>> + if (!cmap_h) {
>> + error_reason = "No cmap handle";
>> + return (-1);
>> + }
>> + if (!default_logfile) {
>> + error_reason = "No default logfile";
>> + return (-1);
>> + }
>> + cmap_handle = cmap_h;
>> + main_logfile = default_logfile;
>> +#endif
>> +
>> if (corosync_main_config_read_logging(error_string) < 0) {
>> error_reason = *error_string;
>> goto parse_error;
>> diff --git a/exec/mainconfig.h b/exec/mainconfig.h
>> index 0c3dce7..30976d4 100644
>> --- a/exec/mainconfig.h
>> +++ b/exec/mainconfig.h
>> @@ -38,6 +38,7 @@
>> #include <corosync/logsys.h>
>> #include <corosync/list.h>
>> #include <corosync/coroapi.h>
>> +#include <corosync/cmap.h>
>>
>> /**
>> * All service handlers
>> @@ -49,7 +50,14 @@ struct dynamic_service {
>> };
>> #define MAX_DYNAMIC_SERVICES 128
>>
>> +#ifdef MAINCONFIG_USE_ICMAP
>> extern int corosync_main_config_read (
>> const char **error_string);
>> +#else
>> +extern int corosync_main_config_read (
>> + cmap_handle_t cmap_h,
>> + const char *default_logfile,
>> + const char **error_string);
>> +#endif
>>
>> #endif /* MAINCONFIG_H_DEFINED */
>
_______________________________________________
discuss mailing list
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss
