Hi Shuai,
Thanks for the patches!
It looks like patch 1/2 is the original patch, and patch 2/2 contains
fixes for the first patch. Please send only one patch containing the
correct code.
The commit heading ("Improve set-log-target functionality") is pretty
vague, so you should add a paragraph that explains how and why it's
being improved. It's also helpful to have a link to the relevant bug in
Bugzilla.
Are you sure you want to be known as "shuai" in the commit log? If you
check the output of "git shortlog -s", you can see that most people use
their full name (properly capitalized), but other styles occur too. I
don't have a problem with it, though, if you indeed prefer just "shuai".
On Fri, 2013-05-10 at 17:05 +0800, shuai wrote:
> ---
> src/daemon/daemon-conf.c | 74 ++++++-------------
> src/daemon/daemon-conf.h | 2 +-
> src/daemon/main.c | 16 +++--
> src/pulsecore/cli-command.c | 48 +++++++------
> src/pulsecore/log.c | 171 ++++++++++++++++++++++++++++++++------------
> src/pulsecore/log.h | 20 ++++--
> src/utils/pactl.c | 18 +----
> 7 files changed, 202 insertions(+), 147 deletions(-)
>
> diff --git a/src/daemon/daemon-conf.c b/src/daemon/daemon-conf.c
> index 2c43cf9..c6ddd70 100644
> --- a/src/daemon/daemon-conf.c
> +++ b/src/daemon/daemon-conf.c
> @@ -78,7 +78,7 @@ static const pa_daemon_conf default_conf = {
> .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 +172,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_xfree(c->log_target);
Use pa_log_target_free().
> pa_xfree(c->config_file);
> pa_xfree(c);
> }
> @@ -179,64 +180,31 @@ 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")) {
> + else if (pa_streq(string, "syslog"))
> c->auto_log_target = 0;
> - c->log_target = PA_LOG_SYSLOG;
> - } else if (pa_streq(string, "stderr")) {
> + 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);
> - return -1;
> - } 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
> + else if (pa_startswith(string, "file:"))
> + c->auto_log_target = 0;
> + else if (pa_startswith(string, "newfile:"))
> + c->auto_log_target = 0;
> + else {
> + pa_log("Invalid log target : %s, error : %s\n", string, pa_cstrerror(errno));
> + return -1;
> + }
This is parsing code, so this should be in pa_log_parse_target(). The
"auto" check can stay, though, because leaving it for
pa_log_parse_target() would probably have some complications.
Now that c->log_target is a pointer, we could use NULL to mean that
either the user didn't specify any log target or the requested target
was "auto". That way c->auto_log_target becomes unnecessary and can be
removed.
> +
> + log_target = pa_log_parse_target(string);
> +
> + if (log_target)
> + c->log_target = log_target;
> + else
> return -1;
>
> return 0;
> @@ -787,7 +755,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", c->auto_log_target ? "auto" : (c->log_target->type == PA_LOG_SYSLOG ? "syslog" : "stderr"));
This was broken before, and is still broken: this will never print any
other targets than "auto", "syslog" or "stderr". We should have
pa_log_target_snprint() for properly converting a log target to string.
> 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));
> diff --git a/src/daemon/daemon-conf.h b/src/daemon/daemon-conf.h
> index faf2540..68d7e3f 100644
> --- a/src/daemon/daemon-conf.h
> +++ b/src/daemon/daemon-conf.h
> @@ -85,7 +85,7 @@ typedef struct pa_daemon_conf {
> 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..f683e95 100644
> --- a/src/daemon/main.c
> +++ b/src/daemon/main.c
> @@ -499,8 +499,13 @@ int main(int argc, char *argv[]) {
> goto finish;
> }
>
> + /* If parse log_target failed from command line, set log target to default log target */
This comment is not accurate. If parsing fails, the daemon will exit. If
the log target is not specified at all (which is different than failure
during parsing), then log_target will be NULL.
> + if (conf->log_target == NULL)
> + conf->log_target = pa_log_target_new();
It's not a good idea to assign to conf->log_target here, especially if
you remove auto_log_target. The information whether the user specified
some target or not is needed later.
> +
> pa_log_set_level(conf->log_level);
> - pa_log_set_target(conf->auto_log_target ? PA_LOG_STDERR : conf->log_target);
> + pa_log_set_target(conf->log_target);
Previously we set the target to stderr here, now you're setting it to
"default", which seems to be syslog. I don't think there should be any
default target. Instead, pa_log_target_new() should take the target type
and file name as parameters.
> +
> if (conf->log_meta)
> pa_log_set_flags(PA_LOG_PRINT_META, PA_LOG_SET);
> if (conf->log_time)
> @@ -810,8 +815,11 @@ 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->auto_log_target) {
> + pa_log_target_free(conf->log_target);
> + conf->log_target = pa_log_target_new();
> + pa_log_set_target(conf->log_target);
> + }
>
> #ifdef HAVE_SETSID
> if (setsid() < 0) {
> @@ -1042,7 +1050,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 1ec8054..f883dfa 100644
> --- a/src/pulsecore/cli-command.c
> +++ b/src/pulsecore/cli-command.c
> @@ -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,31 +1509,34 @@ 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));
> + 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);
> +
> + pa_assert(log_target);
pa_log_parse_target() is given unfiltered user input, so it's possible
that it fails. An assertion is not appropriate here.
> +
> + if (log_target->type == PA_LOG_NULL)
> + pa_log_set_target(log_target);
> + else if (log_target->type == PA_LOG_SYSLOG)
> + pa_log_set_target(log_target);
> + else if (log_target->type == PA_LOG_STDERR)
> + pa_log_set_target(log_target);
> + else if (log_target->type == PA_LOG_FILE) {
> + if (pa_log_set_target(log_target) < 0) {
> + pa_strbuf_printf(buf, "Failed to set log target : %s, error : %s\n",
> + log_target->file, pa_cstrerror(errno));
> + return -1;
> + }
> + } else if (log_target->type == PA_LOG_NEWFILE) {
> + if (pa_log_set_target(log_target) < 0) {
> + pa_strbuf_printf(buf, "Failed to set log target : %s, errno : %s\n",
> + log_target->file, 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;
> }
This last branch is not necessary.
Instead of many "else ifs", you could use a switch statement. It's not
only for style reasons, a switch statement has the benefit that the
compiler will warn if the code doesn't check all possible log target
types, in case the enumeration is ever extended with new types.
The error messages assume that pa_log_set_target() always fails with a
sensible errno set. That doesn't seem to be the case. One option for
fixing this would be to print just "Failed to set log target" without
any further details. That would also make it possible to simplify the
code, because you could replace everything with just four lines:
if (pa_log_set_target(log_target) < 0) {
pa_strbuf_puts(buf, "Failed to set log target.");
return -1;
}
>
> diff --git a/src/pulsecore/log.c b/src/pulsecore/log.c
> index 1aa8b0e..a2b9f54 100644
> --- a/src/pulsecore/log.c
> +++ b/src/pulsecore/log.c
> @@ -48,6 +48,7 @@
>
Patch 1/2 added fcntl.h include here. Out of curiosity, what is that
needed for?
> #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>
> @@ -67,7 +68,9 @@
> #define PA_LOG_MAX_SUFFIX_NUMBER 100
This define is not visible outside log.c. Private identifiers like this
should not use the "PA_" prefix.
>
> 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_log_target_type_t target = PA_LOG_STDERR, target_override;*/
Don't leave commented out code in patches.
> 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;
> @@ -115,10 +118,61 @@ 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) {
> + pa_assert(t);
>
> - target = t;
> + if (t->type == PA_LOG_SYSLOG) {
A switch statement would be good here.
> + target.type = t->type;
> + target.file = t->file;
This would be shorter:
target = *t;
Also, you can do the assignment at the end of the function instead of
repeating it for every target type.
> + } else if (t->type == PA_LOG_STDERR) {
> + target.type = t->type;
> + target.file = t->file;
> + } else if (t->type == PA_LOG_FILE) {
> + int fd = -1;
> +
> + /* Open target file with user rights */
> + if ((fd = open(t->file, O_RDWR|O_TRUNC|O_CREAT, S_IRUSR | S_IWUSR)) >= 0) {
pa_open_cloexec() should be used always instead of raw open().
I don't think we need to have read access to the log file, so O_WRONLY
instead of O_RDWR would make sense.
> + target.type = t->type;
> + target.file = t->file;
> + pa_log_set_fd(fd);
> + } else {
> + printf("Failed to open target file %s, error : %s\n", t->file, pa_cstrerror(errno));
pa_log() can be used here.
> + return -1;
> + }
> + } else if (t->type == PA_LOG_NEWFILE) {
> + char file_path[512];
I think it would be better to dynamically allocate the file path here,
so that we don't have to care if the path exceeds 512 bytes.
> + int fd = -1;
> + int version = 0;
> + int left_size = 0;
> + char *p;
> +
> + pa_strlcpy(file_path, t->file, 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 &&
> + (fd = open(file_path, O_RDWR|O_TRUNC|O_CREAT|O_EXCL, S_IRUSR | S_IWUSR)) < 0);
Use pa_open_cloexec() instead of open(). O_WRONLY instead of O_RDWR
would be good here also.
> +
> + 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);
pa_log() can be used here.
> + return -1;
> + } else {
> + printf("Opened target file %s\n", file_path);
pa_log_debug() can be used here.
> + target.type = t->type;
> + target.file = t->file;
> + pa_log_set_fd(fd);
> + }
> + } else
> + return -1;
This should never happen, so an assertion would be better than returning
-1. There's a bug, though: PA_LOG_NULL is not covered by this code. If
you convert the code to use a switch statement, though, these things
should not be any problem anymore.
> +
> + return 0;
> }
>
> void pa_log_set_flags(pa_log_flags_t _flags, pa_log_merge_t merge) {
> @@ -278,7 +332,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;
> @@ -292,7 +346,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;
> @@ -412,7 +466,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];
>
> @@ -476,57 +531,81 @@ pa_bool_t pa_log_ratelimit(pa_log_level_t level) {
> return pa_ratelimit_test(&ratelimit, level);
> }
>
> -void pa_log_parse_target(const char *string) {
> +pa_log_target *pa_log_target_new(void) {
> + pa_log_target *t = NULL;
> +
> + t = pa_xmalloc(sizeof(pa_log_target));
The convention is to use pa_xnew() or pa_xnew0() when allocating
structs.
> +
> + pa_assert(t);
You don't need to check if pa_xmalloc() or pa_xnew() succeed. They will
abort the whole process if the memory allocation fails.
> +
> + t->type = PA_LOG_SYSLOG;
> + t->file = NULL;
> +
> + return t;
> +}
> +
> +pa_log_target *pa_log_parse_target(const char *string) {
> + pa_log_target *t = NULL;
> +
> pa_assert(string);
>
> - if (pa_streq(string, "auto"))
> - ; /* TODO: what to do here ? */
> - else if (pa_streq(string, "syslog"))
> - pa_log_set_target(PA_LOG_SYSLOG);
> - else if (pa_streq(string, "stderr"))
> - pa_log_set_target(PA_LOG_STDERR);
> - else if (pa_streq(string, "file:")) {
> + t = pa_log_target_new();
I recommend calling pa_log_target_new() only after you have successfully
parsed the target type and the file name. If the parsing fails, the
memory allocation is avoided (and it's easier to avoid memory leaks too
- this code leaks t if parsing fails).
> +
> + if (pa_streq(string, "auto")) {
> + t->type = PA_LOG_STDERR;
> + t->file = NULL;
> +
> + return t;
I don't think pa_log_parse_target() should parse "auto", because log.c
doesn't have enough context to know how it should be handled.
PA_LOG_STDERR is not always the right choice.
> + } else if (pa_streq(string, "null")) {
> + t->type = PA_LOG_NULL;
> + t->file = NULL;
> +
> + return t;
> + } else if (pa_streq(string, "syslog")) {
> + t->type = PA_LOG_SYSLOG;
> + t->file = NULL;
> +
> + return t;
> + } else if (pa_streq(string, "stderr")) {
> + t->type = PA_LOG_STDERR;
> + t->file = NULL;
> +
> + return t;
> + } else if (pa_startswith(string, "file:")) {
> char file_path[512];
No need to have a fixed-size buffer. With dynamic allocation we can
support any path length.
> + char *file = NULL;
>
> 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) {
> - pa_log_set_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;
> - }
> + file = pa_xmalloc(strlen(file_path) + 1);
> + pa_strlcpy(file, file_path, sizeof(file_path));
> +
> + t->type = PA_LOG_FILE;
> + t->file = file;
>
> - } else if (pa_streq(string, "newfile:")) {
> + return t;
> + } else if (pa_startswith(string, "newfile:")) {
> char file_path[512];
> - int version = 0;
> - int left_size;
> - char *p;
> + char *file = NULL;
>
> 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);
> + file = pa_xmalloc(strlen(file_path) + 1);
> + pa_strlcpy(file, file_path, sizeof(file_path));
>
> - 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);
> + t->type = PA_LOG_NEWFILE;
> + t->file = file;
>
> - 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);
> - return;
> - } else {
> - printf("Opened target file %s\n", file_path);
> - pa_log_set_target(PA_LOG_FD);
> - pa_log_set_fd(log_fd);
> - }
> + return t;
> } else
> - return;
> + return NULL;
> +}
> +
> +void pa_log_target_free(pa_log_target *t) {
> + pa_assert(t);
> +
> + pa_xfree(t);
t->file needs to be freed too.
> + printf("log-target-free\n");
Forgot to remove debug prints?
> +
> + t = NULL;
It's not forbidden to have redundant NULL assignments after freeing
something, but this is pretty pointless.
> }
> diff --git a/src/pulsecore/log.h b/src/pulsecore/log.h
> index 69d86d7..e776e60 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 file descriptor, e.g. a char device */
> + PA_LOG_NEWFILE, /* to a file descriptor */
"to a file descriptor" made some sense with PA_LOG_FD, but with FILE and
NEWFILE it's not very helpful. Better: "to a user specified file" ("with
an automatic suffix to avoid overwriting anything").
> PA_LOG_TARGET_MAX
> -} pa_log_target_t;
> +} pa_log_target_type_t; /* original name: pa_log_target_t */
There's no need to keep track of old names.
>
> typedef enum pa_log_level {
> PA_LOG_ERROR = 0, /* Error messages */
> @@ -63,11 +64,16 @@ typedef enum pa_log_merge {
> PA_LOG_RESET
> } pa_log_merge_t;
>
> +typedef struct {
> + pa_log_target_type_t type;
> + char *file;
> +} pa_log_target;
> +
> /* Set an identification for the current daemon. Used when logging to syslog. */
> void pa_log_set_ident(const char *p);
>
> /* Set a log target. */
> -void pa_log_set_target(pa_log_target_t t);
> +int pa_log_set_target(pa_log_target *p);
>
> /* Maximal log level */
> void pa_log_set_level(pa_log_level_t l);
> @@ -109,7 +115,11 @@ void pa_log_levelv(
> const char *format,
> va_list ap);
>
> -void pa_log_parse_target(const char *string);
> +pa_log_target *pa_log_target_new(void);
> +
> +pa_log_target *pa_log_parse_target(const char *string);
> +
> +void pa_log_target_free(pa_log_target *t);
new() and free() functions are usually grouped together (this applies
also to log.c).
--
Tanu
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
