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
