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

[Becker, VincentX]

>-----Original Message-----
>From: Maarten Bosmans [mailto:mkbosmans at gmail.com]
>Sent: Wednesday, February 23, 2011 3:50 PM
>To: General PulseAudio Discussion
>Cc: Becker, VincentX
>Subject: Re: [pulseaudio-discuss] [PATCH] Add a target to the PA log
>feature and improve PA log core
>
>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

Thanks for your review Maarten. So you suggest to split this into several patches (2). One for the outer implementation and the other for the inner one. I will try to attend to the irc meeting tomorrow so I can catch your remarks in real time.

Cheers,
V.

>
>> Vincent BECKER
>> Intel - Wireless System Integration Engineer
>> Medfield platform
>>
>>>
>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
>>
>>
---------------------------------------------------------------------
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.