<div style="line-height:1.7;color:#000000;font-size:14px;font-family:arial"> <div class="moz-cite-prefix">Hi Tanu, On 06/11/2013 12:22 AM, Tanu Kaskinen wrote: </div> <blockquote cite="mid:1370881344.2639.50.camel@dev-laptop" type="cite"> <pre wrap="">On Mon, 2013-05-20 at 23:49 +0800, Shuai Fan wrote: </pre> <blockquote type="cite"> <pre wrap="">--- 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; </pre> </blockquote> <pre wrap="">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.</pre> </blockquote> 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 ... <blockquote cite="mid:1370881344.2639.50.camel@dev-laptop" type="cite"> <blockquote type="cite"> <pre wrap="">- } 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;</pre> </blockquote> </blockquote> ... should be moved. <blockquote cite="mid:1370881344.2639.50.camel@dev-laptop" type="cite"> <blockquote type="cite"> <pre wrap=""> 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); </pre> </blockquote> <pre wrap="">These last five lines are redundant, because... </pre> <blockquote type="cite"><pre wrap="">+ + do { + memset(p, 0, left_size); + + if (version > 0) + pa_snprintf(p, left_size, ".%d", version); </pre></blockquote> <pre wrap="">...it's easier to do just file_path = pa_sprintf_malloc("%s.%d", t->file, version);</pre> </blockquote> 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. <pre wrap="">+ 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;</pre> 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: <pre wrap=""> if (version > LOG_MAX_SUFFIX_NUMBER) {</pre> to <pre wrap=""> if (version >= LOG_MAX_SUFFIX_NUMBER) { </pre> 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. <blockquote cite="mid:1370881344.2639.50.camel@dev-laptop" type="cite"> <blockquote type="cite"><pre wrap="">+ } 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; </pre></blockquote> <pre wrap="">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. </pre> <blockquote type="cite"><pre wrap="">+ } - target = t; </pre></blockquote> <pre wrap="">You could still have target = *t here, instead of duplicating it in every branch of the switch statement. </pre> <blockquote type="cite"> <pre wrap="">+ return 0; } void pa_log_set_flags(pa_log_flags_t _flags, pa_log_merge_t merge) { </pre> </blockquote> </blockquote> Best wishes, Shuai </div>