On Mon, 2013-05-20 at 23:49 +0800, Shuai Fan wrote:
> ---
Last time I complained that the commit message didn't properly explain
what improvements to the set-log-target functionality are being done. It
seems that you put that information to the cover letter. It really
should be in the patch commit message, not in the cover letter, because
the cover letter is not saved in git.
Why are there still two patches instead of just one, with the same
commit message in both?
> 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
> @@ -73,12 +73,11 @@ static const pa_daemon_conf default_conf = {
> .flat_volumes = TRUE,
> .exit_idle_time = 20,
> .scache_idle_time = 20,
> - .auto_log_target = 1,
> .script_commands = NULL,
> .dl_search_path = NULL,
> .load_default_script_file = TRUE,
> .default_script_file = NULL,
> - .log_target = PA_LOG_SYSLOG,
> + .log_target = NULL,
> .log_level = PA_LOG_NOTICE,
> .log_backtrace = 0,
> .log_meta = FALSE,
> @@ -172,6 +171,7 @@ void pa_daemon_conf_free(pa_daemon_conf *c) {
> pa_xfree(c->script_commands);
> pa_xfree(c->dl_search_path);
> pa_xfree(c->default_script_file);
> + pa_log_target_free(c->log_target);
c->log_target can be NULL, so you should check that here.
> pa_xfree(c->config_file);
> pa_xfree(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.
> - } 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;
>
> return 0;
> }
> @@ -755,6 +711,7 @@ char *pa_daemon_conf_dump(pa_daemon_conf *c) {
>
> pa_strbuf *s;
> char cm[PA_CHANNEL_MAP_SNPRINT_MAX];
> + char *p;
I would prefer some more descriptive variable name. Like "log_target",
for example.
>
> pa_assert(c);
>
> @@ -765,6 +722,8 @@ char *pa_daemon_conf_dump(pa_daemon_conf *c) {
>
> pa_assert(c->log_level < PA_LOG_LEVEL_MAX);
>
> + p = pa_log_target_snprint(c->log_target);
So pa_log_target_snprint() allocates a new string? The "snprint" name is
used with functions that take a preallocated (usually in the stack)
buffer. I should have explained this when I asked you to write
"pa_log_target_snprint()", sorry for not doing that.
Allocating a new string is perhaps better in this case, though, because
the log file name can in theory be pretty long, so preallocating a big
enough buffer to cover all possible file names perhaps isn't so good
idea. Just change the function name to pa_log_target_to_string().
c->log_target can be NULL, so you need to check that before calling
pa_log_target_to_string(). That in turn makes it necessary to initialize
p to NULL so that it won't get used in an uninitialized state later.
> +
> pa_strbuf_printf(s, "daemonize = %s\n", pa_yes_no(c->daemonize));
> pa_strbuf_printf(s, "fail = %s\n", pa_yes_no(c->fail));
> pa_strbuf_printf(s, "high-priority = %s\n", pa_yes_no(c->high_priority));
> @@ -787,7 +746,7 @@ char *pa_daemon_conf_dump(pa_daemon_conf *c) {
> pa_strbuf_printf(s, "dl-search-path = %s\n", pa_strempty(c->dl_search_path));
> pa_strbuf_printf(s, "default-script-file = %s\n", pa_strempty(pa_daemon_conf_get_default_script_file(c)));
> pa_strbuf_printf(s, "load-default-script-file = %s\n", pa_yes_no(c->load_default_script_file));
> - pa_strbuf_printf(s, "log-target = %s\n", c->auto_log_target ? "auto" : (c->log_target == PA_LOG_SYSLOG ? "syslog" : "stderr"));
> + pa_strbuf_printf(s, "log-target = %s\n", pa_strempty(p));
> pa_strbuf_printf(s, "log-level = %s\n", log_level_to_string[c->log_level]);
> pa_strbuf_printf(s, "resample-method = %s\n", pa_resample_method_to_string(c->resample_method));
> pa_strbuf_printf(s, "enable-remixing = %s\n", pa_yes_no(!c->disable_remixing));
> @@ -846,5 +805,7 @@ char *pa_daemon_conf_dump(pa_daemon_conf *c) {
> #endif
> #endif
>
> + pa_xfree(p);
> +
> return pa_strbuf_tostring_free(s);
> }
> diff --git a/src/daemon/daemon-conf.h b/src/daemon/daemon-conf.h
> index faf2540..067ff21 100644
> --- a/src/daemon/daemon-conf.h
> +++ b/src/daemon/daemon-conf.h
> @@ -80,12 +80,11 @@ typedef struct pa_daemon_conf {
> pa_server_type_t local_server_type;
> int exit_idle_time,
> scache_idle_time,
> - auto_log_target,
> realtime_priority,
> nice_level,
> resample_method;
> char *script_commands, *dl_search_path, *default_script_file;
> - pa_log_target_t log_target;
> + pa_log_target *log_target;
> pa_log_level_t log_level;
> unsigned log_backtrace;
> char *config_file;
> diff --git a/src/daemon/main.c b/src/daemon/main.c
> index c18524f..0c9ed17 100644
> --- a/src/daemon/main.c
> +++ b/src/daemon/main.c
> @@ -499,8 +499,11 @@ int main(int argc, char *argv[]) {
> goto finish;
> }
>
> + if (!conf->log_target)
> + conf->log_target = pa_log_target_new(PA_LOG_STDERR, NULL);
You should not set conf->log_target here, because you need later the
information whether automatic log target selection should be used.
> +
> + pa_log_set_target(conf->log_target);
> pa_log_set_level(conf->log_level);
> - pa_log_set_target(conf->auto_log_target ? PA_LOG_STDERR : conf->log_target);
> if (conf->log_meta)
> pa_log_set_flags(PA_LOG_PRINT_META, PA_LOG_SET);
> if (conf->log_time)
> @@ -810,8 +813,8 @@ int main(int argc, char *argv[]) {
> daemon_pipe[0] = -1;
> #endif
>
> - if (conf->auto_log_target)
> - pa_log_set_target(PA_LOG_SYSLOG);
> + if (conf->log_target)
> + pa_log_set_target(conf->log_target);
This is not equivalent to the old code. You need to set the log target
to PA_LOG_SYSLOG if conf->log_target is NULL.
>
> #ifdef HAVE_SETSID
> if (setsid() < 0) {
> @@ -1042,7 +1045,7 @@ int main(int argc, char *argv[]) {
> c->cpu_info.cpu_type = PA_CPU_X86;
> if (pa_cpu_init_arm(&(c->cpu_info.flags.arm)))
> c->cpu_info.cpu_type = PA_CPU_ARM;
> - pa_cpu_init_orc(c->cpu_info);
> + pa_cpu_init_orc(c->cpu_info);
> }
>
> pa_assert_se(pa_signal_init(pa_mainloop_get_api(mainloop)) == 0);
> diff --git a/src/pulsecore/cli-command.c b/src/pulsecore/cli-command.c
> index f5489d6..3f57101 100644
> --- a/src/pulsecore/cli-command.c
> +++ b/src/pulsecore/cli-command.c
> @@ -188,7 +188,7 @@ static const struct command commands[] = {
> { "kill-client", pa_cli_command_kill_client, "Kill a client (args: index)", 2},
> { "kill-sink-input", pa_cli_command_kill_sink_input, "Kill a sink input (args: index)", 2},
> { "kill-source-output", pa_cli_command_kill_source_output, "Kill a source output (args: index)", 2},
> - { "set-log-target", pa_cli_command_log_target, "Change the log target (args: null,auto,syslog,stderr,file:PATH)", 2},
> + { "set-log-target", pa_cli_command_log_target, "Change the log target (args: null,auto,syslog,stderr,file:PATH,newfile:PATH)", 2},
> { "set-log-level", pa_cli_command_log_level, "Change the log level (args: numeric level)", 2},
> { "set-log-meta", pa_cli_command_log_meta, "Show source code location in log messages (args: bool)", 2},
> { "set-log-time", pa_cli_command_log_time, "Show timestamps in log messages (args: bool)", 2},
> @@ -1501,6 +1501,7 @@ static int pa_cli_command_suspend(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, p
>
> static int pa_cli_command_log_target(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, pa_bool_t *fail) {
> const char *m;
> + pa_log_target *log_target = NULL;
>
> pa_core_assert_ref(c);
> pa_assert(t);
> @@ -1508,34 +1509,30 @@ static int pa_cli_command_log_target(pa_core *c, pa_tokenizer *t, pa_strbuf *buf
> pa_assert(fail);
>
> if (!(m = pa_tokenizer_get(t, 1))) {
> - pa_strbuf_puts(buf, "You need to specify a log target (null,auto,syslog,stderr,file:PATH).\n");
> - return -1;
> - }
> -
> - if (pa_streq(m, "null"))
> - pa_log_set_target(PA_LOG_NULL);
> - else if (pa_streq(m, "syslog"))
> - pa_log_set_target(PA_LOG_SYSLOG);
> - else if (pa_streq(m, "stderr") || pa_streq(m, "auto")) {
> - /* 'auto' is actually the effect with 'stderr' */
> - pa_log_set_target(PA_LOG_STDERR);
> - } else if (pa_startswith(m, "file:")) {
> - const char *file_path = m + 5;
> - int log_fd;
> -
> - /* Open target file with user rights */
> - if ((log_fd = open(file_path, O_RDWR|O_TRUNC|O_CREAT, S_IRUSR | S_IWUSR)) >= 0) {
> - pa_log_set_target(PA_LOG_FD);
> - pa_log_set_fd(log_fd);
> - } else {
> - pa_strbuf_printf(buf, "Failed to open target file %s, error : %s\n", file_path, pa_cstrerror(errno));
> - return -1;
> - }
> - } else {
> - pa_strbuf_puts(buf, "You need to specify a log target (null,auto,syslog,stderr,file:PATH).\n");
> + pa_strbuf_puts(buf, "You need to specify a log target (null,auto,syslog,stderr,file:PATH,newfile:PATH).\n");
> return -1;
> }
>
> + log_target = pa_log_parse_target(m);
> +
> + switch (log_target->type) {
> + case PA_LOG_STDERR:
> + case PA_LOG_SYSLOG:
> + case PA_LOG_NULL:
> + pa_log_set_target(log_target);
> + break;
> + case PA_LOG_FILE:
> + case PA_LOG_NEWFILE:
> + if (pa_log_set_target(log_target) < 0) {
> + pa_strbuf_puts(buf, "Failed to set log target.");
> + return -1;
This code was changed in the second patch, but this bug is still there:
if setting the log target fails, you don't free log_target.
> + }
> + break;
> + default:
> + pa_strbuf_puts(buf, "You need to specify a log target (null,auto,syslog,stderr,file:PATH,newfile:PATH).\n");
> + return -1;
> + }
> +
> 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
> @@ -29,6 +29,7 @@
> #include <unistd.h>
> #include <string.h>
> #include <errno.h>
> +#include <fcntl.h>
>
> #ifdef HAVE_EXECINFO_H
> #include <execinfo.h>
> @@ -47,9 +48,11 @@
>
> #include <pulsecore/macro.h>
> #include <pulsecore/core-util.h>
> +#include <pulsecore/core-error.h>
> #include <pulsecore/once.h>
> #include <pulsecore/ratelimit.h>
> #include <pulsecore/thread.h>
> +#include <pulsecore/i18n.h>
>
> #include "log.h"
>
> @@ -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);
> + } 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) {
> @@ -276,7 +333,7 @@ void pa_log_levelv_meta(
> char *t, *n;
> int saved_errno = errno;
> char *bt = NULL;
> - pa_log_target_t _target;
> + pa_log_target_type_t _target;
> pa_log_level_t _maximum_level;
> unsigned _show_backtrace;
> pa_log_flags_t _flags;
> @@ -290,7 +347,7 @@ void pa_log_levelv_meta(
>
> init_defaults();
>
> - _target = target_override_set ? target_override : target;
> + _target = target_override_set ? target_override : target.type;
> _maximum_level = PA_MAX(maximum_level, maximum_level_override);
> _show_backtrace = PA_MAX(show_backtrace, show_backtrace_override);
> _flags = flags | flags_override;
> @@ -410,7 +467,8 @@ void pa_log_levelv_meta(
> }
> #endif
>
> - case PA_LOG_FD: {
> + case PA_LOG_FILE:
> + case PA_LOG_NEWFILE: {
> if (log_fd >= 0) {
> char metadata[256];
>
> @@ -473,3 +531,92 @@ pa_bool_t pa_log_ratelimit(pa_log_level_t level) {
>
> return pa_ratelimit_test(&ratelimit, level);
> }
> +
> +pa_log_target *pa_log_target_new(pa_log_target_type_t type, char *file) {
> + pa_log_target *t = NULL;
> +
> + t = pa_xnew(pa_log_target, 1);
> +
> + t->type = type;
> + t->file = file;
pa_xstrdup() needs to be used for the file.
> +
> + return t;
> +}
> +
> +void pa_log_target_free(pa_log_target *t) {
> + pa_assert(t);
> +
> + pa_xfree(t->file);
> + pa_xfree(t);
> +}
> +
> +pa_log_target *pa_log_parse_target(const char *string) {
> + pa_log_target *t = NULL;
> +
> + pa_assert(string);
> +
> + if (pa_streq(string, "stderr"))
> + t = pa_log_target_new(PA_LOG_STDERR, NULL);
> + else if (pa_streq(string, "syslog"))
> + t = pa_log_target_new(PA_LOG_SYSLOG, NULL);
> + else if (pa_streq(string, "null"))
> + t = pa_log_target_new(PA_LOG_NULL, NULL);
> + else if (pa_startswith(string, "file:")) {
> + char *file = NULL;
> + int length = 0;
> +
> + length = strlen(string) - 5;
> + file = pa_xmalloc(length + 1);
> + pa_strlcpy(file, string + 5, length + 1);
> +
> + t = pa_log_target_new(PA_LOG_FILE, file);
This is unnecessarily complicated. This works fine:
t = pa_log_target_new(PA_LOG_FILE, string + 5);
> + } else if (pa_startswith(string, "newfile:")) {
> + char *file = NULL;
> + int length = 0;
> +
> + length = strlen(string) - 8;
> + file = pa_xmalloc(length + 1);
> + pa_strlcpy(file, string + 8, length + 1);
> +
> + t = pa_log_target_new(PA_LOG_NEWFILE, file);
Same as above.
> + } else
> + pa_log(_("Invalid log target."));
> +
> + return t;
> +}
> +
> +char *pa_log_target_snprint(const pa_log_target *t) {
> + char *string = NULL;
> + int length = 0;
> +
> + pa_assert(t);
> +
> + switch (t->type) {
> + case PA_LOG_STDERR:
> + length = strlen("stderr") + 1;
> + string = pa_xmalloc(length);
> + pa_strlcpy(string, "stderr", length);
This is simpler:
string = pa_xstrdup("stderr");
> + break;
> + case PA_LOG_SYSLOG:
> + length = strlen("syslog") + 1;
> + string = pa_xmalloc(length);
> + pa_strlcpy(string, "syslog", length);
> + break;
> + case PA_LOG_NULL:
> + length = strlen("null") + 1;
> + string = pa_xmalloc(length);
> + pa_strlcpy(string, "null", length);
> + break;
> + case PA_LOG_FILE:
> + case PA_LOG_NEWFILE:
> + length = strlen(t->file) + 1;
> + string = pa_xmalloc(length);
> + pa_strlcpy(string, t->file, length);
You need to include the "file:" and "newfile:" part in the string.
string = pa_sprintf_malloc("file:%s", t->file);
> + break;
> + default:
> + printf("Invalid log target\n");
> + break;
The default section isn't necessary here either, and interferes with the
compiler's ability to give useful warnings.
> + }
> +
> + return string;
> +}
> diff --git a/src/pulsecore/log.h b/src/pulsecore/log.h
> index 8dd056b..a52acb8 100644
> --- a/src/pulsecore/log.h
> +++ b/src/pulsecore/log.h
> @@ -32,13 +32,14 @@
> /* A simple logging subsystem */
>
> /* Where to log to */
> -typedef enum pa_log_target {
> +typedef enum pa_log_target_type {
> PA_LOG_STDERR, /* default */
> PA_LOG_SYSLOG,
> PA_LOG_NULL, /* to /dev/null */
> - PA_LOG_FD, /* to a file descriptor, e.g. a char device */
> + PA_LOG_FILE, /* to a user specified file */
> + PA_LOG_NEWFILE, /* with an automatic suffix to avoid overwriting anything */
Please keep the comments aligned.
--
Tanu
