I clicked "Reply" instead of "Reply to All", So I forward my reply mail to mailing list again. <br>Sorry for the noises.<div><br><div class="gmail_quote">---------- 已转发邮件 ----------<br>发件人： <b class="gmail_sendername">rong deng</b> <span dir="ltr"><<a href="mailto:dzrongg@gmail.com">dzrongg@gmail.com</a>></span><br> 日期： 2012年4月3日上午10:41<br>主题： Re: [pulseaudio-discuss] [PATCH] proof of concept: use pactl log to get the buffer log<br>收件人： Tanu Kaskinen <<a href="mailto:tanuk@iki.fi">tanuk@iki.fi</a>><br><br><br>Hi Tanu,<div><br> </div><div>Thanks for your detailed comments, my reply is inline:</div><div><br><div class="gmail_quote">在 2012年4月3日上午1:07，Tanu Kaskinen <span dir="ltr"><<a href="mailto:tanuk@iki.fi" target="_blank">tanuk@iki.fi</a>></span>写道：<br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Deng[1],<br> <br></blockquote><div><br></div><div>About the chinese name, it's OK to call me either Deng or Zhenrong. :)</div><div>Generally speaking, the family name is often matched with Mr/Miss.</div><div class="im"><div><br></div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Very nice patch, I'm impressed! If it weren't for the threading issues,<br> it's almost merge-quality; I found only minor things to complain about.<br> Or well, there seems to be a quite bad bug in get_log_buffer(), but that<br> should be easy to fix.<br> <br> Have you tried if it works with a full buffer? One megabyte sounds like<br> something that the tagstruct system might reject. I didn't find any code<br> that would limit the tag size, though, except for proplists, so probably<br> it will work just fine. (Sidenote: if there are no string size<br> limitations in the protocol, I think that's a bug, because clients can<br> fool the server, and vice versa, into allocating unlimited amounts of<br> memory.)<br> <br></blockquote><div><br></div></div><div>To be honest, I haven't tried a full buffer yet, I waited a few minutes and hoped there's enough stuff in the log buffer, and then I did 'pactl log' to check what's in it. I'm so thrilled when I see some correct output from this command, so I sent this patch out. :D</div> <div class="im"> <div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> As you say, the code isn't really safe to be used from multiple threads.<br> It's not obvious to me how this should be solved, because logging from<br> realtime threads should be lock-free. Needs some thinking...<br> <br></blockquote><div><br></div></div><div>I'm thinking about it: how about this?</div><div>1. we add another thread called log thread, and this thread solely handles the operations of putting stuff into log buffer and getting data out of it.</div> <div>2. the interface between this log thread and other thread is maintained like some message systems. Other thread simply sends the log message to log thread and go on with its own work.</div><div><br></div><div>Do you think it makes some senses?</div> <div class="im"> <div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Please read [2], it contains documentation about the coding style rules.<br> <br></blockquote><div><br></div></div><div>Thanks for your references, I'll follow the coding styles in my later patches.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> [1] Do you prefer to be called "Deng" or "Zhengrong" (or something<br> else)? AFAIK Chinese names have different conventions than western<br> names, so I don't know if it's correct to just pick the first name...<br> <br> [2] <a href="http://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/CodingStyle" target="_blank">http://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/CodingStyle</a><br> <br> -- Tanu<br></div>[...]<br></blockquote><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div> > +    memcpy(result, &log_buffer[log_start_index], log_used_length);<br> > +    result[log_used_length] = '\0';<br> > +<br> > +    return result;<br> > +}<br> <br> </div>Shouldn't this copy the buffer in two parts: from log_start_index to the<br> end of the buffer, and then from the beginning of the buffer to<br> log_end_index?<br> <div><br></div></blockquote><div><br></div></div><div>Nice catch! It should be corrected.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div> > +<br> > +static void write_to_circular_buffer(const char *p, size_t len) {<br> > +    const char *base = p;<br> > +<br> > +    while (len > 0) {<br> <br> </div>Instead of looping, wouldn't it make sense to just write the tail of the<br> log message if it can't fully fit in the buffer? The end result would<br> anyway be the same.<br> <br></blockquote><div><br></div></div><div>You're right, we could do some optimization here to only write the last part of log message when the overall length is bigger than log buffer.</div><div class="im"><div> </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > +        size_t first_chunk;<br> <br> I'd prefer "first_chunk_len".<br> <div><br> > +        size_t min = len;<br> <br> </div>I don't quite get this variable name. But if you get rid of the looping,<br> this shouldn't be needed anyway.<br> <div><br></div></blockquote><div><br></div></div><div>In this version, it's intended to ensure the length copied is not bigger than the buffer's length. When we do the optimization above, this could be eliminated.</div> <div class="im"> <div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br></div><div> > +char *get_log_buffer();<br> <br> </div>Should have pa_log prefix, i.e. pa_log_get_buffer(). Oh, another thing<br> came to my mind: I believe it's not a good idea to declare functions<br> with an empty argument list, because that gets translated (for legacy<br> reasons) to "...", i.e. any number of arguments. The declaration should<br> be pa_log_get_buffer(void);<br> <br></blockquote><div><br></div></div><div>I've never thought of the translation issue, good to know.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Also, pa_log_get_buffer_copy(void) would make it more obvious that the<br> function is returning a new string that the caller has to free.<br></blockquote><div><br></div></div><div>Thanks for your suggestion and gives me the idea of how a function name should be in pulseaudio.  </div></div><br> </div> </div><br></div>