On reflection, the first patch was poorly conceived. My thinking at the time
is explained below but I now feel that only the logging issue is worth
further discussion.
On 30 September 2011 08:00, Max Kellermann <m...@duempel.org> wrote:
> On 2011/09/29 15:03, Steven Blackburn <st...@beeka.org> wrote:
> > I hope the new patches address your concerns. Let me know what you
> think.
>
> "Free the client input buffer if it exists" - Why this? Does it fix a
> memory leak, or is it just refactoring?
> "Tweak logic in httpd_client_read() so that peer disconnection" - is
> that the part that moves the "state==RESPONSE" check into the
> "switch"? How does it detect peer disconnection? What is the
> advantage here? I don't understand the intention of that patch
> either.
>
These two are related (with hindsight, mentioning the free-ing just confuses
things). At the time I first started looking into why the Naim wouldn't talk
with MPD, the httpd_client_read function assumed any call when the client
state was RESPONSE was "unexpected input" so would not get as far as the
status handling switch. I can now see that this was fixed in an earlier
patch but I approached the problem separately as I wasn't using git at the
time - I didn't assume I would produce a patch when I started. My thinking
was that the client state is only relevant if the status was NORMAL, so I
moved the check there. However the code that changes the client state to
RESPONSE also frees fifo buffers so fifo_buffer_write was failing after I
moved the check. To address this, I removed the freeing of the fifo buffer
and changed httpd_client_free() to free the client buffer if it exists,
regardless of client state.
So that was my approach. The one in the master repo does it differently but
achieves the same effect.
>
> "Add more logging to aid debugging communication problems" - I would
> not log "peer disconnected", and even less as a "warning". It's a
> normal event, and if you run a big radio station, your log gets filled
> with these boring messages. The user (non-developer) will probably
> not know what "fifo is not empty" means. And I would not log verbatim
> (unquoted) text received from the socket; I would fear it could be
> used, for example, to put ANSI codes on the admin's terminal, clearing
> log lines with warnings about break-in attempts.
I was a user of MPD before I created a patch and it was the lack of logging
relevant to my issue that led me to modify the code and make some changes to
figure out what the problem was. While "fifo not empty" may not mean much to
the average user, neither did "unexpected input". I was just trying to find
out what the Uniti had sent to MPD that it didn't like and which end was to
blame. Logging "peer disconnected" at least indicated that the Uniti was the
one giving up.
I raised the issue of logging the text received when I first submitted the
mega-patch, expecting some feedback on alternative approaches. I was
thinking more of security in the ways you suggest. I would suggest
sanitizing it but I get the impression that you are not keen on adding this
type of logging.
Some systems have variable logging levels - is this something glib supports,
e.g. enabling debug messages when requested? This would help address your
concerns about log messages becoming large and unwieldy. How interesting a
message is depends on your circumstances: often a high level of operational
detail is fine when things are working perfectly. Even under
perfect conditions, an operator of a big radio station is likely to need
*some* information to support listeners / generate usage reports. Web
servers generate logs of each access and I expect operators of popular web
sites find a way to manage the large logs so the information is there when
they need it rather than turn the logging off completely.
> I do not agree with
> "Unhandled status from read" either, because adding a "default:" will
> suppress gcc warnings about missing "case" statements. Not dealing
> with a documented IO status is a bug, and if you want to detect such a
> bug, write an assertion instead of a log message.
>
I accept the "unhandled status from read" message is not needed... I have
not used glib before and I assumed the glib read() was like recv(), where
the expected error codes are a subset of the system error codes. I.e. one
would normally have a case statement for the expected codes but it the api
does not limit the codes to the expected subset so adding a default is good
practice. I can see now that the glib read returns an enum and so the
default is not a great idea.
>
> (Since you combined three different changes in one patch, I cannot
> pick just one patch and merge it - we have to get all of it right at
> the same time)
>
> I have merged your DLNA patch, but I have fixed indentation and I have
> removed the g_warning() calls, because I wanted to discuss how we
> should do that:
>
> - should not be g_warning(), because it is an informational message
>
I have since found the g_message() function in glib. I just copied from the
code around it at the time - I was more focused on confirming expected
operation of my change.
>
> - is logging an event about a client useful if you cannot identify the
> client? It doesn't show the IP address or any other identification
> of the client.
>
For my uses it was useful as I only had one client. I would welcome a
fuller set of logging with IP address / port number but I just added the
minimum I needed.
On a related note: is it possible to log mpd client connections /
transactions (i.e. the MPD protocol, rather the the httpd streaming)? And
should the httpd streaming use the same logging code / format?
>
> If we do want to log client events, we should assign each client an
> id/name (could be his IP address, or a serial number), and use this
> string in each log message.
>
> Max
>
> P.S.: try "stgit" for refining patches. It's a wonderful tool.
>
Regards,
Steve.
------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure contains a
definitive record of customers, application performance, security
threats, fraudulent activity and more. Splunk takes this data and makes
sense of it. Business sense. IT sense. Common sense.
http://p.sf.net/sfu/splunk-d2d-oct
_______________________________________________
Musicpd-dev-team mailing list
Musicpd-dev-team@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
