[pulseaudio-discuss] [PATCH 1/2] Log: Set log target improvement
Shuai Fan
shuai900217 at 126.com
Sun Jun 16 00:42:17 PDT 2013
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>
More information about the pulseaudio-discuss
mailing list