[pulseaudio-discuss] [PATCH] Add a target to the PA log feature and improve PA log core

[Maarten Bosmans]

Here's my review of that patch. It will be handled in the meeting
tomorrow, but I'm sure I've forgotten by then, so I sent it to the
list.

2010/11/22 Becker, VincentX <vincentx.becker at intel.com>:
> Hi all,
>
> Could you please review this patch ?
> This is a patch that adds a generic log target for PA log (log.c). This new target can be either a char device or a regular file. For our remote log device needs, it has been necessary to directly log Pulseaudio traces to a file descriptor.
>
> Also optimizations in the memory allocations have been done to reuse local pointers to prevent extra memory allocations (on stack or heap), that would happen in case of very big trace buffers, and that could potentially have an impact on audio latency in embedded systems.
>
> Modifications :
>
> src/daemon/cmdline.c : Update Pulseaudio help and error message for the 'log-target' command argument.

The current help message has a width of 80 characters. It would be
nice to keep it that way and find another way to fit the new
log-target argument in there.

> src/daemon/daemon-conf.c : Add the condition when the target given is a file name given with relative or absolute path. If the file already exists, it creates a new file that will contain the current date and time inserted in the name. For this purpose, two static utilities functions have been added. The file descriptor is then passed to the PA log in pulsecore.

I don't think the renaming with the data appended to the filename is
functionality that belongs in pulseaudio. If you want serious logging,
use syslog, if you want quick-and-dirty, use the filename. Pulse
should just append or overwrite the file, either is fine.

Also, as daemon-conf opens the file, it should call pa_close itself
too. Then the pa_log_close_fd function can be removed.

> src/pulsecore/log.c : The function pa_log_levelv_meta formats the text data and sends it to the appropriate target. There are 2 main changes :
> 1) Add the target PA_LOG_FILED which allows to write the formatted log data to a file descriptor output. Add functions to open and close that file descriptor.

This would better be called PA_LOG_FD

> 2) the algorithms around 'char *t' have been reordered in order to optimize memory use. This could be useful when traces are big. The trace buffer "char text" has been set to 16*1024Kb which allocates already 16Kb on the stack. This buffer is then copied into t, in the for loop that checks for carriage returns. What needed to be optimized is that extra memory is allocated in case of metadata (location and timestamp) is prepended to t, by creating dynamically a new buffer. The idea is to prepend the data directly into t (and append if it's the case) before we affect the value of 'text'. It avoids one dynamic memory allocation, at least in the case of the new target PA_LOG_FILED.
> Therefore, a 'metadata' buffer is created and prepended in t whatever the target. One switch/case is actually added to build this metadata buffer and we keep the other one just for write the actual log (text+metadata) in the target.

Please separate this change out in another patch. That way it can be
reviewed better.

> P.S. : there are bad indentations due to tabs. I use emacs and tried the indentation Lisp function given here but without success : http://www.pulseaudio.org/wiki/CodingStyle  . It keeps putting tabs instead of spaces ! How could I solve that ?
>
> Thanks,
>
> Vincent

Maarten

> Vincent BECKER
> Intel - Wireless System Integration Engineer
> Medfield platform
>
>
> ---------------------------------------------------------------------
> Intel Corporation SAS (French simplified joint stock company)
> Registered headquarters: "Les Montalets"- 2, rue de Paris,
> 92196 Meudon Cedex, France
> Registration Number:  302 456 199 R.C.S. NANTERRE
> Capital: 4,572,000 Euros
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

Ah, to bad you send it to a public list then. I guess we're all
criminals now ;-)

> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at mail.0pointer.de
> https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
>
>