Improve the "set log target" functionality

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2013-04-17 at 22:03 +0800, Shuai Fan wrote:
> Hello all,
>      I'm interested in contributing to PulseAudio through GSOC2013. 
> These days, I follow the guide of GSOC page on PulseAudio. And find this 
> bug in bug list:
>      https://bugs.freedesktop.org/show_bug.cgi?id=58389
>      I just thought, maybe I should try something easier to start, 
> rather the big idea "Better JACK Configurability". Then, I tried to 
> solve this problem.
>      My solution is:
>      Add function "pa_log_parse_target" to "log.c" to parse parameters, 
> and call "pa_log_set_target" to set target. Then extend the file 
> "pactl.c" to support "set-log-target".
> 
>      Could you give me some suggestions about this solution?
> 
>      The code is:

A proper patch sent with git send-email would have been nice
(instructions now available at [1]). But no worries, it's easy enough to
deal with the formatting used here. Comments below.

[1] http://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail

> log.c:
> 
> diff --git a/src/pulsecore/log.c b/src/pulsecore/log.c
> index 8eaef54..1aa8b0e 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>
> @@ -63,6 +64,7 @@
>   #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 PA_LOG_MAX_SUFFIX_NUMBER 100
> 
>   static char *ident = NULL; /* in local charset format */
>   static pa_log_target_t target = PA_LOG_STDERR, target_override;
> @@ -473,3 +475,58 @@ 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_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:")) {
> +        char file_path[512];
> +
> +        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;
> +        }
> +
> +    } else if (pa_streq(string, "newfile:")) {
> +        char file_path[512];
> +        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;
> +        } else {
> +            printf("Opened target file %s\n", file_path);
> +            pa_log_set_target(PA_LOG_FD);
> +            pa_log_set_fd(log_fd);
> +        }
> +    } else
> +        return;
> +}

I don't think pa_log_parse_target() should actually set the target, it
should only do the parsing, and return a struct representing the parsed
data:

typedef struct {
    pa_log_target_type_t type;
    char *file;
} pa_log_target;

I propose that the current pa_log_target_t enum gets renamed to
pa_log_target_type_t, because the pa_log_target struct would otherwise
have too similar name. I think the PA_LOG_FD log target should also be
replaced with PA_LOG_FILE and PA_LOG_NEWFILE targets, so this takes some
more refactoring.

pa_log_set_target() should then be changed so that it takes a
pa_log_target pointer instead of just the log target type. I think the
file opening should be done in pa_log_set_target() instead of in the
calling code, like is currently done.

> log.h:
> 
> diff --git a/src/pulsecore/log.h b/src/pulsecore/log.h
> index 8dd056b..69d86d7 100644
> --- a/src/pulsecore/log.h
> +++ b/src/pulsecore/log.h
> @@ -109,6 +109,8 @@ void pa_log_levelv(
>           const char *format,
>           va_list ap);
> 
> +void pa_log_parse_target(const char *string);
> +
>   #if __STDC_VERSION__ >= 199901L
> 
>   /* ISO varargs available */
> 
> 
> 
> pactl.c:
> 
> diff --git a/src/utils/pactl.c b/src/utils/pactl.c
> index 0fb62cb..6b17ade 100644
> --- a/src/utils/pactl.c
> +++ b/src/utils/pactl.c
> @@ -127,7 +127,8 @@ static enum {
>       SET_SOURCE_OUTPUT_MUTE,
>       SET_SINK_FORMATS,
>       SET_PORT_LATENCY_OFFSET,
> -    SUBSCRIBE
> +    SUBSCRIBE,
> +    SET_LOG_TARGET
>   } action = NONE;
> 
>   static void quit(int ret) {
> @@ -1391,7 +1392,9 @@ static void context_state_callback(pa_context *c, 
> void *userdata) {
>                                                 NULL,
>                                                 NULL));
>                       break;
> -
> +                case SET_LOG_TARGET:
> +                    /* Target has been set in main*/
> +                    break;
>                   default:
>                       pa_assert_not_reached();
>               }
> @@ -1510,6 +1513,7 @@ static void help(const char *argv0) {
>       printf("%s %s %s %s\n", argv0, _("[options]"), "set-sink-formats", 
> _("#N FORMATS"));
>       printf("%s %s %s %s\n", argv0, _("[options]"), 
> "set-port-latency-offset", _("CARD-NAME|CARD-#N PORT OFFSET"));
>       printf("%s %s %s\n",    argv0, _("[options]"), "subscribe");
> +    printf("%s %s %s %s\n", argv0, _("[options]"), "set-log-target", 
> _("TARGET"));
>       printf(_("\nThe special names @DEFAULT_SINK@, @DEFAULT_SOURCE@ and 
> @DEFAULT_MONITOR@\n"
>                "can be used to specify the default sink, source and 
> monitor.\n"));
> 
> @@ -1978,6 +1982,16 @@ int main(int argc, char *argv[]) {
>                   goto quit;
>               }
> 
> +        } else if (pa_streq(argv[optind], "set-log-target")) {
> +            action = SET_LOG_TARGET;
> +
> +            if (argc != optind+2) {
> +                pa_log(_("You have to specify a log target"));
> +                goto quit;
> +            }
> +
> +            pa_log_parse_target(argv[optind+1]);
> +
>           } else if (pa_streq(argv[optind], "help")) {
>               help(bn);
>               ret = 0;

Calling pa_log_parse_target() in pactl sets the log target only for the
pactl process. The goal is to set the target in the pulseaudio server
process. This requires extending the native protocol. That's a bit
complicated, so you could skip pactl for now. After the other stuff is
done, we can start working on the protocol extension if you want.

-- 
Tanu



[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux