Thanks for your comments. More inline...
> -----Original Message-----
> From: Worley, Dale (BL60:9D30)
> Sent: Wednesday, October 22, 2008 3:38 PM
> To: Beeton, Carolyn (CAR:9D60)
> Cc: [EMAIL PROTECTED]
> Subject: Re: [sipX-dev] Please review stdout/stderr intercept
> patch onXECS-1423
>
> On Mon, 2008-10-20 at 15:02 -0400, Carolyn Beeton wrote:
> > I've attached patches which implement the pipes described in
> > http://track.sipfoundry.org/browse/XECS-1423 for stdout and stderr.
> > The first patch is the OS component, which intercepts the output in
> > such a way that the application can (optionally) get it
> while it waits
> > for the child to terminate. The second patch is the
> application level
> > code to ask for the terminal output, and log it. The
> result is a much
> > cleaner look when running from a terminal. All output to stdout is
> > logged (along with the service name) in sipxsupervisor.log at level
> > PRI_INFO, and stderr output with PRI_ERR.
>
> Some questions:
>
> 1. Is the capturing of stdout/stderr mandatory? Or can I use
> OsProcess in a way that is like before? Does the default
> behavior change?
It is not mandatory that the app capture stdout/stderr, but I am going
to remove the old way of redirecting it to a file. Default behaviour
does not change, in that the app has to explicitly ask for output.
>
> 2. The comment on getOutput isn't so clear. Perhaps this is an
> improvement:
>
> /// Wait until the process produces output on stdout or
> stderr, or /// exits. Returns -1 if process exits. If
> output is obtained, /// sets 'message' to the output and 'fd'
> to STDOUT_FILENO or STDERR_FILENO /// depending on which fd
> output was obtained from, then returns >0.
> /// 'message' values may be broken at arbitrary points.
> virtual int getOutput(int& fd, UtlString& message) = 0;
I am thinking of changing it to a "push" method instead of requiring the
application to "pull" the messages. So the app would call
OsProcess::setCaptureOutput(OsCallback handleStdoutMessage, OsCallback
handleStderrMsg)
once before launch. The wait function would then call select in a loop
if the callbacks were set, and invoke them to pass the message to the
app.
OR
another option (since Scott didn't like the first getOutput) would be to
return two strings, rather than the fd (which is a bit weird, I'll
admin). So it would look like
int getOutput(UtlString& stdoutMsg, UtlString& stderrMsg)
and you could get one or the other, or both, but getting neither would
indicate the child process was gone.
Any preference?
>
> 3. There are a number of places where printf's are commented
> out. These should be converted into a suitable #ifdef.
yep
>
> 4. The placement of the null is incorrect here:
>
> + // read until all input has been read
> + rc = read(realFd, buf, sizeof(buf)-1);
> + buf[sizeof(buf)-1]=0;
>
> as it will be incorrect if rc < sizeof(buf)-1.
>
> 5. But I think we can omit placing a null at all using code like:
>
> printf("read %d bytes more: %.*s\n", rc, rc, buf);
>
> message.append(buf, rc);
I'll try again, but I don't think it works without adding the nulls
because if we return from the read with a full buffer, it won't be
null-terminated and then append doesn't do the right thing. I test with
the buffer size set very low so I can see what it does.
>
> 6. I think the main loop of OsProcessLinux::getOutput could
> be simplified to something like:
>
> // read until all input has been read
> message.remove(0);
> do
> {
> rc = read(realFd, buf, sizeof(buf)-1);
> if (rc > 0)
> {
> // printf("read %d bytes: %.*s\n", rc, rc, buf);
> message.append(buf, rc);
> }
> } while (rc == sizeof(buf)-1);
>
> // If output was obtained, return its length.
> // But if no output was obtained, return the 0 or -1 that
> // read() returned.
> if (message.length() > 0)
> {
> rc = message.length();
> }
>
> 7. There might be some ugly behavior if the process closes
> stdout or stderr significantly before it exits. From the man
> pages, I would expect select() to always return ready-to-read
> on the other end of the closed pipe, and read() on it to
> return 0 (EOF), so getOutput would always return immediately.
Thank you! I did not manage to understand that from the man pages, and
could not figure out why it was returning 2 with no data to read, after
the child exited. Now I see that that is correct, and that exiting from
the select loop is the right thing to do if there is no data to read on
either fd.
>
> Dale
>
>
>
I will resubmit a patch for a second review in a couple of days.
Carolyn
_______________________________________________
sipx-dev mailing list
[email protected]
List Archive: http://list.sipfoundry.org/archive/sipx-dev
Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev
