[PATCH 1/2] Log: Set log target improvement

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Tanu,

On 06/11/2013 12:22 AM, Tanu Kaskinen wrote:

On Mon, 2013-05-20 at 23:49 +0800, Shuai Fan wrote:

---

 src/daemon/daemon-conf.c    |  75 +++++----------------
 src/daemon/daemon-conf.h    |   3 +-
 src/daemon/main.c           |  11 +--
 src/pulsecore/cli-command.c |  49 +++++++-------
 src/pulsecore/log.c         | 161 ++++++++++++++++++++++++++++++++++++++++++--
 src/pulsecore/log.h         |  22 ++++--
 6 files changed, 221 insertions(+), 100 deletions(-)

diff --git a/src/daemon/daemon-conf.c b/src/daemon/daemon-conf.c
index f1e5476..7726c90 100644
--- a/src/daemon/daemon-conf.c
+++ b/src/daemon/daemon-conf.c
@@ -179,65 +179,21 @@ void pa_daemon_conf_free(pa_daemon_conf *c) {
 #define PA_LOG_MAX_SUFFIX_NUMBER 100
 
 int pa_daemon_conf_set_log_target(pa_daemon_conf *c, const char *string) {
+    pa_log_target *log_target = NULL;
+
     pa_assert(c);
     pa_assert(string);
 
-    if (pa_streq(string, "auto"))
-        c->auto_log_target = 1;
-    else if (pa_streq(string, "syslog")) {
-        c->auto_log_target = 0;
-        c->log_target = PA_LOG_SYSLOG;
-    } else if (pa_streq(string, "stderr")) {
-        c->auto_log_target = 0;
-        c->log_target = PA_LOG_STDERR;
-    } else if (pa_startswith(string, "file:")) {
-        char file_path[512];
-        int log_fd;
-
-        pa_strlcpy(file_path, string + 5, sizeof(file_path));
-
-        /* Open target file with user rights */
-        if ((log_fd = open(file_path, O_RDWR|O_TRUNC|O_CREAT, S_IRUSR | S_IWUSR)) >= 0) {
-             c->auto_log_target = 0;
-             c->log_target = PA_LOG_FD;
-             pa_log_set_fd(log_fd);
-        } else {
-            printf("Failed to open target file %s, error : %s\n", file_path, pa_cstrerror(errno));
-            return -1;
-        }
-    } else if (pa_startswith(string, "newfile:")) {
-        char file_path[512];
-        int log_fd;
-        int version = 0;
-        int left_size;
-        char *p;
-
-        pa_strlcpy(file_path, string + 8, sizeof(file_path));
-
-        left_size = sizeof(file_path) - strlen(file_path);
-        p = file_path + strlen(file_path);
-
-        do {
-            memset(p, 0, left_size);
-
-            if (version > 0)
-                pa_snprintf(p, left_size, ".%d", version);
-        } while (++version <= PA_LOG_MAX_SUFFIX_NUMBER &&
-                 (log_fd = open(file_path, O_RDWR|O_TRUNC|O_CREAT|O_EXCL, S_IRUSR | S_IWUSR)) < 0);
-
-        if (version > PA_LOG_MAX_SUFFIX_NUMBER) {
-            memset(p, 0, left_size);
-            printf("Tried to open target files '%s', '%s.1', '%s.2' ... '%s.%d', but all failed.\n",
-                   file_path, file_path, file_path, file_path, PA_LOG_MAX_SUFFIX_NUMBER - 1);
+    if (!pa_streq(string, "auto")) {
+        log_target = pa_log_parse_target(string);
+
+        if (!log_target) {
+            c->log_target = log_target;
             return -1;

If a function fails, it should not have any side effects (if possible).
Here you're setting c->log_target to NULL if
pa_daemon_conf_set_log_target() fails. That probably doesn't have
problems in practice, but it's not good style.
I'm sorry for doing that. I think it works like this:

if (!pa_streq(string, "auto")) {                           
    log_target = pa_log_parse_target(string);                   
}                                                            
                                                                     
if (log_target)                                                   
    c->log_target = log_target;


and  the the following ...

-        } else {
-            printf("Opened target file %s\n", file_path);
-            c->auto_log_target = 0;
-            c->log_target = PA_LOG_FD;
-            pa_log_set_fd(log_fd);
         }
-    } else
-        return -1;
+    }
+
+    c->log_target = log_target;
...  should be moved.

 
     return 0;
 }
diff --git a/src/pulsecore/log.c b/src/pulsecore/log.c
index 8eaef54..f95031a 100644
--- a/src/pulsecore/log.c
+++ b/src/pulsecore/log.c
@@ -63,9 +66,11 @@
 #define ENV_LOG_BACKTRACE "PULSE_LOG_BACKTRACE"
 #define ENV_LOG_BACKTRACE_SKIP "PULSE_LOG_BACKTRACE_SKIP"
 #define ENV_LOG_NO_RATELIMIT "PULSE_LOG_NO_RATE_LIMIT"
+#define LOG_MAX_SUFFIX_NUMBER 100
 
 static char *ident = NULL; /* in local charset format */
-static pa_log_target_t target = PA_LOG_STDERR, target_override;
+static pa_log_target target = { PA_LOG_STDERR, NULL };
+static pa_log_target_type_t target_override;
 static pa_bool_t target_override_set = FALSE;
 static pa_log_level_t maximum_level = PA_LOG_ERROR, maximum_level_override = PA_LOG_ERROR;
 static unsigned show_backtrace = 0, show_backtrace_override = 0, skip_backtrace = 0;
@@ -113,10 +118,62 @@ void pa_log_set_level(pa_log_level_t l) {
     maximum_level = l;
 }
 
-void pa_log_set_target(pa_log_target_t t) {
-    pa_assert(t < PA_LOG_TARGET_MAX);
+int pa_log_set_target(pa_log_target *t) {
+    char *file_path = NULL;
+    int length = 0;
+    int fd = -1;
+    int version = 0;
+    int left_size = 0;
+    char *p = NULL;
+
+    pa_assert(t);
+
+    switch (t->type) {
+        case PA_LOG_STDERR:
+        case PA_LOG_SYSLOG:
+        case PA_LOG_NULL:
+            target = *t;
+            break;
+        case PA_LOG_FILE:
+            if ((fd = pa_open_cloexec(t->file, O_WRONLY | O_TRUNC | O_CREAT, S_IRUSR | S_IWUSR)) >= 0) {
+                target = *t;
+                pa_log_set_fd(fd);
+            } else {
+                pa_log(_("Failed to open target file '%s'."), t->file);
+                return -1;
+            }
+            break;
+        case PA_LOG_NEWFILE:
+            left_size = LOG_MAX_SUFFIX_NUMBER / 10 + 2;
+            length = strlen(t->file) + left_size;
+            file_path = pa_xmalloc(length);
+            pa_strlcpy(file_path, t->file, length);
+            p = file_path + strlen(t->file);

These last five lines are redundant, because...


+
+            do {
+                memset(p, 0, left_size);
+
+                if (version > 0)
+                    pa_snprintf(p, left_size, ".%d", version);

...it's easier to do just

    file_path = pa_sprintf_malloc("%s.%d", t->file, version);

Yes, it is shorter.

I tried this:

    file_path = pa_sprintf_malloc("%s.%d", t->file, version); 
    left_size = strlen(file_path) - strlen(t->file) + 1;      
    p = file_path + strlen(t->file);                         
                                                               
    do {                                                      
        memset(p, 0, left_size);                              
                                                         
        if (version > 0)                                      
            pa_snprintf(p, left_size, ".%d", version);        
    } while (++version < LOG_MAX_SUFFIX_NUMBER &&             
                (fd = pa_open_cloexec(file_path, O_WRONLY | O_TRUNC | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR)) < 0);


It seems better and works fine.

I found a bug in this function(? or maybe I misunderstood it ?). Currently, LOG_MAX_SUFFIX_NUMBER is set to 100.
    #define LOG_MAX_SUFFIX_NUMBER 100
Then I changed it to 5 in order to test the number of new_log_target files.
    #define LOG_MAX_SUFFIX_NUMBER 5
and use command "set-log-target newfile:/home/shuai/newlog.txt" in src/pacmd, 6 times(to see the error message).

Then, I got newlog.txt, newlog.txt.1, newlog.txt.2, newlog.txt.3. That is to say, only 4 files, and the max suffix is only 3.

The following code is the same(Fixed something which has noting to do with this stuff).

In the above while loop:

when version == 4, file_path = "/home/shuai/newlog.txt.4"
but when ++version, version = 5, so (++version < LOG_MAX_SUFFIX_NUMBER) failed.
so, the log-target "/home/shuai/newlog.txt.4" not opened(created). And fd < 0, so, the log target "*.txt.4" not be set. And the loop is ended.

And, of course, the version > LOG_MAX_SUFFIX_NUMBER test will always false. So, the error message(log message) will not be showed.

+            if (version > LOG_MAX_SUFFIX_NUMBER) {
+                memset(p, 0, left_size);
+                pa_log(_("Tried to open target file '%s', '%s.1', '%s.2' ... '%s.%d', but all failed."),
+                        file_path, file_path, file_path, file_path, LOG_MAX_SUFFIX_NUMBER - 1);
+		 pa_xfree(file_path);
+                return -1;
+            } else {
+                pa_log_debug("Opened target file %s\n", file_path);
+                pa_log_set_fd(fd);
+            }
+            break;

I think there are 2 ways to fix this stuff:

1. Change this:
    ++version < LOG_MAX_SUFFIX_NUMBER
to
    version++ < LOG_MAX_SUFFIX_NUMBER

Then there will be 5 log target files(i. e.  *, *.1, *.2, *.3, *.4), and the test will be changed:

	if (version > LOG_MAX_SUFFIX_NUMBER) {
to

	if (version >= LOG_MAX_SUFFIX_NUMBER) {

to show the error message.


2. Change this:
    version++ < LOG_MAX_SUFFIX_NUMBER
to
    version++ <= LOG_MAX_SUFFIX_NUMBER

Then there will be 6 log target files(i. e. *, *.1, *.2, *.3, *.4, and *.5 included), and the largest suffix number is 5.


Which one is better? I prefer the second.



Well, I should have found this bug(?) earlier, the code is copied from other file. It's all my fault.



+            } while (++version < LOG_MAX_SUFFIX_NUMBER &&
+                    (fd = pa_open_cloexec(file_path, O_WRONLY | O_TRUNC | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR)) < 0);
+
+            if (version > LOG_MAX_SUFFIX_NUMBER) {
+                memset(p, 0, left_size);
+                pa_log(_("Tried to open target file '%s', '%s.1', '%s.2' ... '%s.%d', but all failed."),
+                        file_path, file_path, file_path, file_path, LOG_MAX_SUFFIX_NUMBER - 1);
+                return -1;
+            } else {
+                pa_log_debug("Opened target file %s\n", file_path);
+                target = *t;
+                pa_log_set_fd(fd);
+            }
+            break;
+        default:
+            return -1;

Don't bother having a default section. It prevents the compiler from
giving a warning if pa_log_target_type_t is some day extended and this
code is not updated accordingly.


+    }
 
-    target = t;

You could still have target = *t here, instead of duplicating it in
every branch of the switch statement.


+    return 0;
 }
 
 void pa_log_set_flags(pa_log_flags_t _flags, pa_log_merge_t merge) {




Best wishes,
    Shuai
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20130616/68c1d9b9/attachment-0001.html>


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux