Hi,
I haven't tested your patch, but some comments on the code below.
On Tue, 2011-03-08 at 17:15 +0100, Vincent Becker wrote:
> This patch enables logging of text debug messages (pa_log feature) into a file or a device driver.
> Example : pulseaudio --log-target=file:./mylog.txt
>
> Signed-off-by: Vincent Becker <vincentx.becker at intel.com>
> ---
> src/daemon/cmdline.c | 5 +++--
> src/daemon/daemon-conf.c | 34 ++++++++++++++++++++++++++++++++--
> src/pulsecore/log.c | 24 ++++++++++++++++++++++++
> src/pulsecore/log.h | 5 +++++
> 4 files changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/src/daemon/cmdline.c b/src/daemon/cmdline.c
> index f6cdcdc..2c3eb67 100644
> --- a/src/daemon/cmdline.c
> +++ b/src/daemon/cmdline.c
> @@ -145,7 +145,8 @@ void pa_cmdline_help(const char *argv0) {
> " this time passed\n"
> " --log-level[=LEVEL] Increase or set verbosity level\n"
> " -v Increase the verbosity level\n"
> - " --log-target={auto,syslog,stderr} Specify the log target\n"
> + " --log-target={auto,syslog,stderr,\n"
> + " file:PATH} Specify the log target\n"
> " --log-meta[=BOOL] Include code location in log messages\n"
> " --log-time[=BOOL] Include timestamps in log messages\n"
> " --log-backtrace=FRAMES Include a backtrace in log messages\n"
> @@ -318,7 +319,7 @@ int pa_cmdline_parse(pa_daemon_conf *conf, int argc, char *const argv [], int *d
>
> case ARG_LOG_TARGET:
> if (pa_daemon_conf_set_log_target(conf, optarg) < 0) {
> - pa_log(_("Invalid log target: use either 'syslog', 'stderr' or 'auto'."));
> + pa_log(_("Invalid log target: use either 'syslog', 'stderr', 'auto' or a valid file name 'file:<path>'."));
> goto fail;
> }
> break;
> diff --git a/src/daemon/daemon-conf.c b/src/daemon/daemon-conf.c
> index e38e67a..5696f00 100644
> --- a/src/daemon/daemon-conf.c
> +++ b/src/daemon/daemon-conf.c
> @@ -28,6 +28,8 @@
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
>
> #ifdef HAVE_SCHED_H
> #include <sched.h>
> @@ -141,6 +143,9 @@ static const pa_daemon_conf default_conf = {
> #endif
> };
>
> +static int log_fd = -1;
> +
> +
> pa_daemon_conf* pa_daemon_conf_new(void) {
> pa_daemon_conf *c;
>
> @@ -166,6 +171,9 @@ pa_daemon_conf* pa_daemon_conf_new(void) {
>
> void pa_daemon_conf_free(pa_daemon_conf *c) {
> pa_assert(c);
> + if (log_fd >= 0)
> + pa_close(log_fd);
> +
> pa_xfree(c->script_commands);
> pa_xfree(c->dl_search_path);
> pa_xfree(c->default_script_file);
> @@ -185,8 +193,30 @@ int pa_daemon_conf_set_log_target(pa_daemon_conf *c, const char *string) {
> } else if (!strcmp(string, "stderr")) {
> c->auto_log_target = 0;
> c->log_target = PA_LOG_STDERR;
> - } else
> - return -1;
> + } else if (pa_startswith(string, "file:")) {
> + char *file_path;
> + const char *state = NULL;
> + unsigned i;
> +
> + /* After second pa_split call, file_path will contain the right part of file:<path> */
> + for (i = 0; i < 2; i++)
> + file_path = pa_split(string, ":", &state);
I believe this would leak. You could just use file_path = &string[5]
here.
> + /* Check if the file is regular */
> + if ((log_fd = open(file_path, O_RDWR|O_TRUNC|O_CREAT, 0777)) >= 0) {
Definitely don't want to give execute permissions for that file. For
paranoia's sake, I'd use S_IRWXU.
> + 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));
> + pa_xfree(file_path);
> + return -1;
> + }
> + pa_xfree(file_path);
> + }
> + else
> + return -1;
>
> return 0;
> }
> diff --git a/src/pulsecore/log.c b/src/pulsecore/log.c
> index 2c0e267..a5e26c8 100644
> --- a/src/pulsecore/log.c
> +++ b/src/pulsecore/log.c
> @@ -71,6 +71,8 @@ static unsigned show_backtrace = 0, show_backtrace_override = 0, skip_backtrace
> static pa_log_flags_t flags = 0, flags_override = 0;
> static pa_bool_t no_rate_limit = FALSE;
>
> +static int fdlog = -1;
> +
> #ifdef HAVE_SYSLOG_H
> static const int level_to_syslog[] = {
> [PA_LOG_ERROR] = LOG_ERR,
> @@ -128,6 +130,12 @@ void pa_log_set_flags(pa_log_flags_t _flags, pa_log_merge_t merge) {
> flags = _flags;
> }
>
> +void pa_log_set_fd(int fd) {
> + pa_assert(fd >= 0);
> +
> + fdlog = fd;
> +}
> +
> void pa_log_set_show_backtrace(unsigned nlevels) {
> show_backtrace = nlevels;
> }
> @@ -399,6 +407,22 @@ void pa_log_levelv_meta(
> }
> #endif
>
> + case PA_LOG_FD: {
> + if (fdlog != -1) {
> + char metadata[256];
> +
> + pa_snprintf(metadata, sizeof(metadata), "\n%c %s%s", level_to_char[level], timestamp, location);
> +
> + if ((write(fdlog, metadata, strlen(metadata)) < 0) || (write(fdlog, t, strlen(t)) < 0)) {
> + saved_errno = errno;
> + pa_close(fdlog);
> + fdlog = -1;
Maybe switch to PA_LOG_STDERR here (or at least print a message, so you
know logging failed (for example when the disk is full).
Also, if we close the fd here, there'll be another (invalid) close at
daemon shutdown, right?
> + }
> + }
> +
> + break;
> + }
> +
> case PA_LOG_NULL:
> default:
> break;
> diff --git a/src/pulsecore/log.h b/src/pulsecore/log.h
> index 1fd38d4..bfe245c 100644
> --- a/src/pulsecore/log.h
> +++ b/src/pulsecore/log.h
> @@ -36,6 +36,7 @@ typedef enum pa_log_target {
> PA_LOG_STDERR, /* default */
> PA_LOG_SYSLOG,
> PA_LOG_NULL, /* to /dev/null */
> + PA_LOG_FD, /* to a file descriptor, e.g. of a char device */
> PA_LOG_TARGET_MAX
> } pa_log_target_t;
>
> @@ -80,6 +81,10 @@ void pa_log_set_show_backtrace(unsigned nlevels);
> /* Skip the first backtrace frames */
> void pa_log_set_skip_backtrace(unsigned nlevels);
>
> +/* Set the file descriptor of the logging device.
> + Daemon conf is in charge of opening this device */
> +void pa_log_set_fd(int fd);
> +
> void pa_log_level_meta(
> pa_log_level_t level,
> const char*file,
Rest seems fine.
Cheers,
Arun
