[pulseaudio-discuss] Fwd: [PATCH] proof of concept: use pactl log to get the buffer log

[rong deng]

I clicked "Reply" instead of "Reply to All", So I forward my reply mail to
mailing list again.
Sorry for the noises.

---------- 已转发邮件 ----------
发件人： rong deng <dzrongg at gmail.com>
日期： 2012年4月3日 上午10:41
主题： Re: [pulseaudio-discuss] [PATCH] proof of concept: use pactl log to get
the buffer log
收件人： Tanu Kaskinen <tanuk at iki.fi>


Hi Tanu,

Thanks for your detailed comments, my reply is inline:

在 2012年4月3日 上午1:07，Tanu Kaskinen <tanuk at iki.fi>写道：

> Hi Deng[1],
>
>
About the chinese name, it's OK to call me either Deng or Zhenrong. :)
Generally speaking, the family name is often matched with Mr/Miss.

Very nice patch, I'm impressed! If it weren't for the threading issues,
> it's almost merge-quality; I found only minor things to complain about.
> Or well, there seems to be a quite bad bug in get_log_buffer(), but that
> should be easy to fix.
>
> Have you tried if it works with a full buffer? One megabyte sounds like
> something that the tagstruct system might reject. I didn't find any code
> that would limit the tag size, though, except for proplists, so probably
> it will work just fine. (Sidenote: if there are no string size
> limitations in the protocol, I think that's a bug, because clients can
> fool the server, and vice versa, into allocating unlimited amounts of
> memory.)
>
>
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

As you say, the code isn't really safe to be used from multiple threads.
> It's not obvious to me how this should be solved, because logging from
> realtime threads should be lock-free. Needs some thinking...
>
>
I'm thinking about it: how about this?
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.
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.

Do you think it makes some senses?


> Please read [2], it contains documentation about the coding style rules.
>
>
Thanks for your references, I'll follow the coding styles in my later
patches.


> [1] Do you prefer to be called "Deng" or "Zhengrong" (or something
> else)? AFAIK Chinese names have different conventions than western
> names, so I don't know if it's correct to just pick the first name...
>
> [2]
> http://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/CodingStyle
>
> -- Tanu
> [...]
>


> > +    memcpy(result, &log_buffer[log_start_index], log_used_length);
> > +    result[log_used_length] = '\0';
> > +
> > +    return result;
> > +}
>
> Shouldn't this copy the buffer in two parts: from log_start_index to the
> end of the buffer, and then from the beginning of the buffer to
> log_end_index?
>
>
Nice catch! It should be corrected.


> > +
> > +static void write_to_circular_buffer(const char *p, size_t len) {
> > +    const char *base = p;
> > +
> > +    while (len > 0) {
>
> Instead of looping, wouldn't it make sense to just write the tail of the
> log message if it can't fully fit in the buffer? The end result would
> anyway be the same.
>
>
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.


> > +        size_t first_chunk;
>
> I'd prefer "first_chunk_len".
>
> > +        size_t min = len;
>
> I don't quite get this variable name. But if you get rid of the looping,
> this shouldn't be needed anyway.
>
>
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.


> > +char *get_log_buffer();
>
> Should have pa_log prefix, i.e. pa_log_get_buffer(). Oh, another thing
> came to my mind: I believe it's not a good idea to declare functions
> with an empty argument list, because that gets translated (for legacy
> reasons) to "...", i.e. any number of arguments. The declaration should
> be pa_log_get_buffer(void);
>
>
I've never thought of the translation issue, good to know.


> Also, pa_log_get_buffer_copy(void) would make it more obvious that the
> function is returning a new string that the caller has to free.
>

Thanks for your suggestion and gives me the idea of how a function name
should be in pulseaudio.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20120403/9c6cc6b5/attachment.htm>