Bug 888

Tim, as you recently have been fixing debugging messages, could you hack at this one too?

Considering recent patches that now invoke previously unseen kderror and kdwarning messages (bug report 1655, Commit 5354555 2013-08-26), addressing this bug report would be helpful toward debugging and resolving what has become too much stdout/stderr spew.

"PID xxxx terminated" provides no useful information. :-)

A more useful message:

PID xxxx terminated (process name/description).

Trivial note: the code is now located in tdelibs/tdeinit/tdeinit.cpp:1354.

> PID xxxx terminated (process name/description).

AFAICT, that is not possible. The fprintf is called when waitpid returns a non-zero value, i.e. when a child process has been terminated.

  exit_pid = waitpid(-1, 0, WNOHANG);

At that point, the child process no longer exists, so it is not possible to get the process name for it. 
Perhaps I am wrong, but I also googled extensively and didn't find anything about this that I didn't already know.

One solution would be to store the process names and pids in a symbol table, but that would be to heavy to do since:
1) we would have to update the symbol table continuously to detect new child processes spawn and process terminated
2) each single update operation requires accessing a file in /proc/$pid/cmdline

Since the message is inside an #ifndef NDEBUG block, you could compile the NDEBUG option to hide the message (and several others as well).

If no one has anything else to say on this, I suggest we close this bug.

Looks like the following snippet has existed since 3.5.10:

#ifndef NDEBUG
  fprintf(stderr, "kdeinit: PID %ld terminated.\n", (long) exit_pid);
#endif

I have a 3.5.10 virtual machine. I don't see these messages. The likely reason is the 3.5.10 packages never were built with debugging enabled and that makes sense.

I only saw the messages after I started participating in the Trinity project.

From a developer's perspective, building packages with debugging enabled makes much sense and is much needed. My problem with this particular message is the message provides nothing useful. Instead the message provides a lot of clutter spew in the user's xsession-error log.

If there is no method to reveal the process name, and I accept as much, then why don't we delete that three-line snippet from the code? Or, if the message is useful in certain development areas (I'd like to learn more), rather than use fprintf, use kdebug such that the message can be disabled using kdebugdialog?

Let's change this to an appropriate kdDebug statement and comment out the resultant line for now.

Tim

Okay. Michele, please let me attempt the patch and you approve. I need to learn. :-)

Created attachment 1660 [details]
Patch to convert fprintf->kdDebug and comment out message

Michele, please provide a sanity check of the patch. I haven't yet tested. Thanks.

Hi Darrell, I would suggest something like this:

#ifndef NDEBUG
  kdDebug() << "[tdeinit] PID " << (long) exit_pid << " terminated.\n" << endl;
#endif

No point to keep the old code commented, since we can trace that from GIT if we need. The code is not commented, since the debug message can be enable/disable by kdebugdialog.
Also please note the space before "terminated" that was missing.

In the same file there are several "#ifndef NDEBUG" blocks that use fprintf to write messages. For consistency, you should change all of them to kdDebug as well

If all fprintf strings are converted to kdDebug:

* Why would we need '#ifndef NDEBUG' qualifiers? Should they be deleted?

* Currently kdDebug() is global whereas kdDebug(xxxx) provides a method to silence specific kdDebug messages. Which kdDebug(xxxx) code should be used?

I don't want to tinker with converting the fprintf->kdDebug. We have bug 1710 to address that issue.

(In reply to comment #9)
> If all fprintf strings are converted to kdDebug:
> 
> * Why would we need '#ifndef NDEBUG' qualifiers? Should they be deleted?
Good point! I should have thought of that too :(

> * Currently kdDebug() is global whereas kdDebug(xxxx) provides a method to
> silence specific kdDebug messages. Which kdDebug(xxxx) code should be used?
I am not on a TDE computer now, but I can check later (or tomorrow).

> I don't want to tinker with converting the fprintf->kdDebug. We have bug 1710
> to address that issue.
Then perhaps we could close this bug and just keep bug 1710 for this issue too.
I agree that fprintf->kdDebug conversions should be done in general and not limited to specific messages or files.
What do you think?

To coordinate everybody's concerns, for closing this report I think we leave the fprintf as an fprintf and just comment out the message. Eventually we convert the fprintf to kdDebug in bug 1710.

I checked the 3.4.3 sources and this specific message existed then too. Been around a while.

When creating packages with debugging enabled, this specific message provides no value that I can see because there is no way to know the process name and there are no previous debug messages about the PID being started. That is, there is no way to coordinate the termination message with anything. Thus the message only adds spew noise rather than usefulness, which is my original concern.

Until working on bug 1710, the proposed patch would look like this:

// #ifndef NDEBUG
//   fprintf(stderr, "[tdeinit] PID %ld terminated.\n", (long) exit_pid);
// #endif

We reduce output spew noise. Sound acceptable?

Created attachment 1668 [details]
Patch to disable single fprintf message and nominal string cleanup

Looks good to me. 
At line 1353 I would just add a comment to say that the fprintf commented out may have to be reinstated when the fprintf -> kdDebug conversion is done.

>At line 1353 I would just add a comment to say that the fprintf commented out
>may have to be reinstated when the fprintf -> kdDebug conversion is done.
Added. Patch pushed to git in commit 5dc0b354.

Thanks for the help!