[pulseaudio-discuss] Improve the "set log target" functionality
Tanu Kaskinen
tanuk at iki.fi
Sun Apr 21 04:46:17 PDT 2013
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
More information about the pulseaudio-discuss
mailing list