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 <[email protected]>: > 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 > [email protected] > https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss > > _______________________________________________ pulseaudio-discuss mailing list [email protected] https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
