Hi everyone.
Based on last week's gyrations I trimmed all unnecessary fat from this
function. This added check seems like unnecessary fat to me as this
function is internal.
But I want this code review to be done, so I will make the following
change, which is in the spirit of what is proposed.
if ((command == NULL) || (stderr_buf == NULL) || (stderr_bufsize<= 0) ||
(stdout_bufsize< 0) || ((stdout_buf != NULL) ^ (stdout_bufsize != 0))) {
return (BE_ERR_INVAL);
}
Note the second line is what it is because stdout_buf can indeed be
passed as NULL.
Thanks,
Jack
On 07/19/10 12:54 PM, Glenn Lagasse wrote:
* Evan Layton ([email protected]) wrote:
Do you think it would be valuable to do some validation on the input
arguments to be_run_cmd?
Maybe check the buf pointers against NULL and the bufsizes against
non-positive ints. What do you think?
I think it is good enough that the comments state what is and isn't
allowed as far as parameters (including that a NULL stderr buffer is not
allowed). Bad parameters passed to be_run_cmd() are programming errors,
and will never come based on an end-user input. As such, I don't think
it is helpful to do such checking.
This could probably be considered a nit but I kind of would prefer
to see at least some checking here. Yes this is internal only but I
would prefer to see us doing this checking. Wouldn't it be better to
return an error for invalid arguments rather than core-dumping
should someone add a call to this in libbe and not correctly set the
arguments. Would it be OK to add something like the following?
if (command == NULL || stderr_buf == NULL || stdout_buf == NULL ||
stderr_bufsize !> 0 || stdout_bufsize !> 0) {
return (BE_ERR_INVAL);
}
I kind of like this idea. Especially since this type of programming
error can't be caught at compile time and could potentially slip through
the cracks to an end user who ends up with a core dump and with no idea
why or what for.
Glenn
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
