On Tue, 2008-10-28 at 10:24 -0400, Carolyn Beeton wrote:
> I have attached new patches to
> http://track.sipfoundry.org/browse/XECS-1423 reflecting comments made.
> It is tested on SuSE10.3 and FC8. Dale and Scott, could you please
> re-review?
When and how getOutput removes the previous contents of *stdoutMsg and
*stderrMsg seems to be awkward. Right now, they're cleared only when
output arrives on the corresponding pipe, which means that it's hard
for the caller to tell which pipe produced output unless it cleared
both *stdoutMsg and *stderrMsg beforehand -- in which case, why have
getOutput clear them?
Also, there is no documentation in the function declaration as to when
*stdoutMsg and *stderrMsg are cleared. (Although there are zillions
of other functions with the same problem, grumble.)
I'd suggest either not having getOutput clear them first, or (better)
always clear them. The easiest way to do that would be to relocate
the remove() calls into
+ if ( stdoutMsg != NULL )
+ {
+ FD_SET(m_fdout[0], &outputFds);
+ }
+ if ( stderrMsg != NULL )
+ {
+ FD_SET(m_fderr[0], &outputFds);
+ }
Some documentation of readAll would be good, especially its return
code.
In this loop:
+ do
+ {
+ rc = read(fd, buf, sizeof(buf)-1);
+ if (rc>0)
+ {
+ message->append(buf, rc);
+ }
+ } while (rc == sizeof(buf)-1);
there is no reason to use "sizeof(buf)-1" rather than "sizeof(buf)",
as we are not appending a NUL.
Once you've dup'ed the writing ends of the pipes to fds 0 and 1, you
should close the other fds they are open on. This probably doesn't
make a difference in practice, but you might as well handle this
cleanly. In this case, you can do it by removing the test inside the
"close all other fds" loop:
+ // replace stdout/stderr with write part of the
pipes
+ dup2(m_fdout[1], STDOUT_FILENO);
+ dup2(m_fderr[1], STDERR_FILENO);
+
+ // close all other file descriptors that may be
open,
+ // but don't close our new copy of stdout and
stderr
int max_fd=1023;
struct rlimit limits ;
if(getrlimit(RLIMIT_NOFILE, &limits) == 0)
{
max_fd = limits.rlim_cur - 1 ;
}
+ for (int i=max_fd; i>2; i--)
{
+ >>>>>>>>>>> if ( (i!=m_fdout[1]) && (i!=m_fderr[1]) )
+ {
+ close(i);
+ }
}
max_fd should not be set to 1023, as that might not be correct.
(Right now, sipX sets the max fd to 16384; see
sipXpbx/etc/init.d/sipxpbx.in line 504.) The manual pages say to use
"sysconf(SC_OPEN_MAX)-1".
In a few places, comments refer to "pipe" in the singular, when in
fact the code creates two pipes.
Dale
_______________________________________________
sipx-dev mailing list
[email protected]
List Archive: http://list.sipfoundry.org/archive/sipx-dev
Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev
