Re: [PATCH 4/5] log: Introduce function pointer to handle different log backends
On 21/06/17 12:58, Torsten Hilbrich wrote:
> This allows for easier additions of other logging mechanism.
>
> Using the syslog loglevel as parameter because we need to be able to
> map the MMLogLevel and the GLogLevelFlags to a common representation
> when using the log_backend in _mm_log and log_handler.
>
> The syslog level is more suitable because it supports more values than
> the MMLogLevel.
Pushed to git master, thanks.
> ---
> src/mm-log.c | 55 ---
> 1 file changed, 36 insertions(+), 19 deletions(-)
>
> diff --git a/src/mm-log.c b/src/mm-log.c
> index 151b40d..fcbef34 100644
> --- a/src/mm-log.c
> +++ b/src/mm-log.c
> @@ -49,6 +49,12 @@ static GTimeVal rel_start = { 0, 0 };
> static int logfd = -1;
> static gboolean append_log_level_text = TRUE;
>
> +static void (*log_backend) (const char *loc,
> +const char *func,
> +int syslog_level,
> +const char *message,
> +size_t length);
> +
> typedef struct {
> guint32 num;
> const char *name;
> @@ -118,6 +124,30 @@ log_level_description (MMLogLevel level)
> return NULL;
> }
>
> +static void
> +log_backend_file (const char *loc,
> + const char *func,
> + int syslog_level,
> + const char *message,
> + size_t length)
> +{
> +ssize_t ign;
> +ign = write (logfd, message, length);
> +if (ign) {} /* whatever; really shut up about unused result */
> +
> +fsync (logfd); /* Make sure output is dumped to disk immediately */
> +}
> +
> +static void
> +log_backend_syslog (const char *loc,
> +const char *func,
> +int syslog_level,
> +const char *message,
> +size_t length)
> +{
> +syslog (syslog_level, "%s", message);
> +}
> +
> void
> _mm_log (const char *loc,
> const char *func,
> @@ -127,7 +157,6 @@ _mm_log (const char *loc,
> {
> va_list args;
> GTimeVal tv;
> -ssize_t ign;
>
> if (!(log_level & level))
> return;
> @@ -169,14 +198,7 @@ _mm_log (const char *loc,
>
> g_string_append_c (msgbuf, '\n');
>
> -if (logfd < 0)
> -syslog (mm_to_syslog_priority (level), "%s", msgbuf->str);
> -else {
> -ign = write (logfd, msgbuf->str, msgbuf->len);
> -if (ign) {} /* whatever; really shut up about unused result */
> -
> -fsync (logfd); /* Make sure output is dumped to disk immediately */
> -}
> +log_backend (loc, func, mm_to_syslog_priority (level), msgbuf->str,
> msgbuf->len);
> }
>
> static void
> @@ -185,14 +207,7 @@ log_handler (const gchar *log_domain,
> const gchar *message,
> gpointer ignored)
> {
> -ssize_t ign;
> -
> -if (logfd < 0)
> -syslog (glib_to_syslog_priority (level), "%s", message);
> -else {
> -ign = write (logfd, message, strlen (message));
> -if (ign) {} /* whatever; really shut up about unused result */
> -}
> +log_backend (NULL, NULL, glib_to_syslog_priority (level), message,
> strlen(message));
> }
>
> gboolean
> @@ -243,9 +258,10 @@ mm_log_setup (const char *level,
> /* Grab start time for relative timestamps */
> g_get_current_time (_start);
>
> -if (log_file == NULL)
> +if (log_file == NULL) {
> openlog (G_LOG_DOMAIN, LOG_CONS | LOG_PID | LOG_PERROR, LOG_DAEMON);
> -else {
> +log_backend = log_backend_syslog;
> +} else {
> logfd = open (log_file,
>O_CREAT | O_APPEND | O_WRONLY,
>S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
> @@ -255,6 +271,7 @@ mm_log_setup (const char *level,
> errno, strerror (errno));
> return FALSE;
> }
> +log_backend = log_backend_file;
> }
>
> g_log_set_handler (G_LOG_DOMAIN,
>
--
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
[PATCH 4/5] log: Introduce function pointer to handle different log backends
This allows for easier additions of other logging mechanism.
Using the syslog loglevel as parameter because we need to be able to
map the MMLogLevel and the GLogLevelFlags to a common representation
when using the log_backend in _mm_log and log_handler.
The syslog level is more suitable because it supports more values than
the MMLogLevel.
---
src/mm-log.c | 55 ---
1 file changed, 36 insertions(+), 19 deletions(-)
diff --git a/src/mm-log.c b/src/mm-log.c
index 2b7907c..b6cd0ba 100644
--- a/src/mm-log.c
+++ b/src/mm-log.c
@@ -50,6 +50,12 @@ static int logfd = -1;
static gboolean func_loc = FALSE;
static gboolean append_log_level_text = TRUE;
+static void (*log_backend) (const char *loc,
+const char *func,
+int syslog_level,
+const char *message,
+size_t length);
+
typedef struct {
guint32 num;
const char *name;
@@ -119,6 +125,30 @@ log_level_description (MMLogLevel level)
return NULL;
}
+static void
+log_backend_file (const char *loc,
+ const char *func,
+ int syslog_level,
+ const char *message,
+ size_t length)
+{
+ssize_t ign;
+ign = write (logfd, message, length);
+if (ign) {} /* whatever; really shut up about unused result */
+
+fsync (logfd); /* Make sure output is dumped to disk immediately */
+}
+
+static void
+log_backend_syslog (const char *loc,
+const char *func,
+int syslog_level,
+const char *message,
+size_t length)
+{
+syslog (syslog_level, "%s", message);
+}
+
void
_mm_log (const char *loc,
const char *func,
@@ -128,7 +158,6 @@ _mm_log (const char *loc,
{
va_list args;
GTimeVal tv;
-ssize_t ign;
if (!(log_level & level))
return;
@@ -169,14 +198,7 @@ _mm_log (const char *loc,
g_string_append_c (msgbuf, '\n');
-if (logfd < 0)
-syslog (mm_to_syslog_priority (level), "%s", msgbuf->str);
-else {
-ign = write (logfd, msgbuf->str, msgbuf->len);
-if (ign) {} /* whatever; really shut up about unused result */
-
-fsync (logfd); /* Make sure output is dumped to disk immediately */
-}
+log_backend (loc, func, mm_to_syslog_priority (level), msgbuf->str,
msgbuf->len);
}
static void
@@ -185,14 +207,7 @@ log_handler (const gchar *log_domain,
const gchar *message,
gpointer ignored)
{
-ssize_t ign;
-
-if (logfd < 0)
-syslog (glib_to_syslog_priority (level), "%s", message);
-else {
-ign = write (logfd, message, strlen (message));
-if (ign) {} /* whatever; really shut up about unused result */
-}
+log_backend (NULL, NULL, glib_to_syslog_priority (level), message,
strlen(message));
}
gboolean
@@ -246,9 +261,10 @@ mm_log_setup (const char *level,
/* Grab start time for relative timestamps */
g_get_current_time (_start);
-if (log_file == NULL)
+if (log_file == NULL) {
openlog (G_LOG_DOMAIN, LOG_CONS | LOG_PID | LOG_PERROR, LOG_DAEMON);
-else {
+log_backend = log_backend_syslog;
+} else {
logfd = open (log_file,
O_CREAT | O_APPEND | O_WRONLY,
S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
@@ -258,6 +274,7 @@ mm_log_setup (const char *level,
errno, strerror (errno));
return FALSE;
}
+log_backend = log_backend_file;
}
g_log_set_handler (G_LOG_DOMAIN,
--
2.7.4
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
[PATCH 4/5] log: Introduce function pointer to handle different log backends
This allows for easier additions of other logging mechanism.
Using the syslog loglevel as parameter because we need to be able to
map the MMLogLevel and the GLogLevelFlags to a common representation
when using the log_backend in _mm_log and log_handler.
The syslog level is more suitable because it supports more values than
the MMLogLevel.
---
src/mm-log.c | 55 ---
1 file changed, 36 insertions(+), 19 deletions(-)
diff --git a/src/mm-log.c b/src/mm-log.c
index 2b7907c..b6cd0ba 100644
--- a/src/mm-log.c
+++ b/src/mm-log.c
@@ -50,6 +50,12 @@ static int logfd = -1;
static gboolean func_loc = FALSE;
static gboolean append_log_level_text = TRUE;
+static void (*log_backend) (const char *loc,
+const char *func,
+int syslog_level,
+const char *message,
+size_t length);
+
typedef struct {
guint32 num;
const char *name;
@@ -119,6 +125,30 @@ log_level_description (MMLogLevel level)
return NULL;
}
+static void
+log_backend_file (const char *loc,
+ const char *func,
+ int syslog_level,
+ const char *message,
+ size_t length)
+{
+ssize_t ign;
+ign = write (logfd, message, length);
+if (ign) {} /* whatever; really shut up about unused result */
+
+fsync (logfd); /* Make sure output is dumped to disk immediately */
+}
+
+static void
+log_backend_syslog (const char *loc,
+const char *func,
+int syslog_level,
+const char *message,
+size_t length)
+{
+syslog (syslog_level, "%s", message);
+}
+
void
_mm_log (const char *loc,
const char *func,
@@ -128,7 +158,6 @@ _mm_log (const char *loc,
{
va_list args;
GTimeVal tv;
-ssize_t ign;
if (!(log_level & level))
return;
@@ -169,14 +198,7 @@ _mm_log (const char *loc,
g_string_append_c (msgbuf, '\n');
-if (logfd < 0)
-syslog (mm_to_syslog_priority (level), "%s", msgbuf->str);
-else {
-ign = write (logfd, msgbuf->str, msgbuf->len);
-if (ign) {} /* whatever; really shut up about unused result */
-
-fsync (logfd); /* Make sure output is dumped to disk immediately */
-}
+log_backend (loc, func, mm_to_syslog_priority (level), msgbuf->str,
msgbuf->len);
}
static void
@@ -185,14 +207,7 @@ log_handler (const gchar *log_domain,
const gchar *message,
gpointer ignored)
{
-ssize_t ign;
-
-if (logfd < 0)
-syslog (glib_to_syslog_priority (level), "%s", message);
-else {
-ign = write (logfd, message, strlen (message));
-if (ign) {} /* whatever; really shut up about unused result */
-}
+log_backend (NULL, NULL, glib_to_syslog_priority (level), message,
strlen(message));
}
gboolean
@@ -246,9 +261,10 @@ mm_log_setup (const char *level,
/* Grab start time for relative timestamps */
g_get_current_time (_start);
-if (log_file == NULL)
+if (log_file == NULL) {
openlog (G_LOG_DOMAIN, LOG_CONS | LOG_PID | LOG_PERROR, LOG_DAEMON);
-else {
+log_backend = log_backend_syslog;
+} else {
logfd = open (log_file,
O_CREAT | O_APPEND | O_WRONLY,
S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
@@ -258,6 +274,7 @@ mm_log_setup (const char *level,
errno, strerror (errno));
return FALSE;
}
+log_backend = log_backend_file;
}
g_log_set_handler (G_LOG_DOMAIN,
--
2.7.4
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [PATCH 4/5] log: Introduce function pointer to handle different log backends
Hey Torsten,
See comments below.
On 22/05/17 07:49, Torsten Hilbrich wrote:
> This allows for easier additions of other logging mechanism.
> ---
> src/mm-log.c | 53 ++---
> 1 file changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/src/mm-log.c b/src/mm-log.c
> index bedf88f..6578d68 100644
> --- a/src/mm-log.c
> +++ b/src/mm-log.c
> @@ -50,6 +50,12 @@ static int logfd = -1;
> static gboolean func_loc = FALSE;
> static gboolean append_log_level_text = TRUE;
>
> +static void (*log_backend) (const char *loc,
> +const char *func,
> +int level,
It isn't clear what kind of enum "level" is. For the looks of it in this patch,
it is the syslog log level, but I believe we should stick to passing to the
backend the MM specific log level (here, again a MMLogLevel enum would help). I
understand that the log level isn't currently used in the file backend proposed
here, but for consistency, I think it makes sense to avoid passing the syslog
log level to every backend.
> +const char *message,
> +size_t length);
> +
> typedef struct {
> guint32 num;
> const char *name;
> @@ -63,6 +69,28 @@ static const LogDesc level_descs[] = {
> { 0, NULL }
> };
>
> +static void log_backend_file (const char *loc,
> + const char *func,
> + int level,
> + const char *message,
> + size_t length)
The "static void" part should go in its own line when the function is defined.
> +{
> +ssize_t ign;
> +ign = write (logfd, message, length);
> +if (ign) {} /* whatever; really shut up about unused result */
> +
> +fsync (logfd); /* Make sure output is dumped to disk immediately */
> +}
> +
> +static void log_backend_syslog (const char *loc,
> +const char *func,
> +int level,
> +const char *message,
> +size_t length)
The "static void" part should go in its own line when the function is defined.
> +{
> +syslog (level, "%s", message);
> +}
> +
> static GString *msgbuf = NULL;
> static volatile gsize msgbuf_once = 0;
>
> @@ -129,7 +157,6 @@ _mm_log (const char *loc,
> {
> va_list args;
> GTimeVal tv;
> -ssize_t ign;
>
> if (!(log_level & level))
> return;
> @@ -170,14 +197,7 @@ _mm_log (const char *loc,
>
> g_string_append_c (msgbuf, '\n');
>
> -if (logfd < 0)
> -syslog (mm_to_syslog_priority (level), "%s", msgbuf->str);
> -else {
> -ign = write (logfd, msgbuf->str, msgbuf->len);
> -if (ign) {} /* whatever; really shut up about unused result */
> -
> -fsync (logfd); /* Make sure output is dumped to disk immediately */
> -}
> +log_backend (loc, func, mm_to_syslog_priority (level), msgbuf->str,
> msgbuf->len);
> }
>
> static void
> @@ -186,14 +206,7 @@ log_handler (const gchar *log_domain,
> const gchar *message,
> gpointer ignored)
> {
> -ssize_t ign;
> -
> -if (logfd < 0)
> -syslog (glib_to_syslog_priority (level), "%s", message);
> -else {
> -ign = write (logfd, message, strlen (message));
> -if (ign) {} /* whatever; really shut up about unused result */
> -}
> +log_backend (NULL, NULL, glib_to_syslog_priority (level), message,
> strlen(message));
> }
>
> gboolean
> @@ -247,9 +260,10 @@ mm_log_setup (const char *level,
> /* Grab start time for relative timestamps */
> g_get_current_time (_start);
>
> -if (log_file == NULL)
> +if (log_file == NULL) {
> openlog (G_LOG_DOMAIN, LOG_CONS | LOG_PID | LOG_PERROR, LOG_DAEMON);
> -else {
> +log_backend = log_backend_syslog;
> +} else {
> logfd = open (log_file,
>O_CREAT | O_APPEND | O_WRONLY,
>S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
> @@ -259,6 +273,7 @@ mm_log_setup (const char *level,
> errno, strerror (errno));
> return FALSE;
> }
> +log_backend = log_backend_file;
> }
>
> g_log_set_handler (G_LOG_DOMAIN,
>
--
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
[PATCH 4/5] log: Introduce function pointer to handle different log backends
This allows for easier additions of other logging mechanism.
---
src/mm-log.c | 53 ++---
1 file changed, 34 insertions(+), 19 deletions(-)
diff --git a/src/mm-log.c b/src/mm-log.c
index bedf88f..6578d68 100644
--- a/src/mm-log.c
+++ b/src/mm-log.c
@@ -50,6 +50,12 @@ static int logfd = -1;
static gboolean func_loc = FALSE;
static gboolean append_log_level_text = TRUE;
+static void (*log_backend) (const char *loc,
+const char *func,
+int level,
+const char *message,
+size_t length);
+
typedef struct {
guint32 num;
const char *name;
@@ -63,6 +69,28 @@ static const LogDesc level_descs[] = {
{ 0, NULL }
};
+static void log_backend_file (const char *loc,
+ const char *func,
+ int level,
+ const char *message,
+ size_t length)
+{
+ssize_t ign;
+ign = write (logfd, message, length);
+if (ign) {} /* whatever; really shut up about unused result */
+
+fsync (logfd); /* Make sure output is dumped to disk immediately */
+}
+
+static void log_backend_syslog (const char *loc,
+const char *func,
+int level,
+const char *message,
+size_t length)
+{
+syslog (level, "%s", message);
+}
+
static GString *msgbuf = NULL;
static volatile gsize msgbuf_once = 0;
@@ -129,7 +157,6 @@ _mm_log (const char *loc,
{
va_list args;
GTimeVal tv;
-ssize_t ign;
if (!(log_level & level))
return;
@@ -170,14 +197,7 @@ _mm_log (const char *loc,
g_string_append_c (msgbuf, '\n');
-if (logfd < 0)
-syslog (mm_to_syslog_priority (level), "%s", msgbuf->str);
-else {
-ign = write (logfd, msgbuf->str, msgbuf->len);
-if (ign) {} /* whatever; really shut up about unused result */
-
-fsync (logfd); /* Make sure output is dumped to disk immediately */
-}
+log_backend (loc, func, mm_to_syslog_priority (level), msgbuf->str,
msgbuf->len);
}
static void
@@ -186,14 +206,7 @@ log_handler (const gchar *log_domain,
const gchar *message,
gpointer ignored)
{
-ssize_t ign;
-
-if (logfd < 0)
-syslog (glib_to_syslog_priority (level), "%s", message);
-else {
-ign = write (logfd, message, strlen (message));
-if (ign) {} /* whatever; really shut up about unused result */
-}
+log_backend (NULL, NULL, glib_to_syslog_priority (level), message,
strlen(message));
}
gboolean
@@ -247,9 +260,10 @@ mm_log_setup (const char *level,
/* Grab start time for relative timestamps */
g_get_current_time (_start);
-if (log_file == NULL)
+if (log_file == NULL) {
openlog (G_LOG_DOMAIN, LOG_CONS | LOG_PID | LOG_PERROR, LOG_DAEMON);
-else {
+log_backend = log_backend_syslog;
+} else {
logfd = open (log_file,
O_CREAT | O_APPEND | O_WRONLY,
S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
@@ -259,6 +273,7 @@ mm_log_setup (const char *level,
errno, strerror (errno));
return FALSE;
}
+log_backend = log_backend_file;
}
g_log_set_handler (G_LOG_DOMAIN,
--
2.7.4
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
