[pulseaudio-discuss] [PATCH 1/2] Log feature:Add a new log target to a file descriptor

Arun Raghavan arun.raghavan at collabora.co.uk
Tue Mar 8 09:54:30 PST 2011


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




More information about the pulseaudio-discuss mailing list