Hi Joe, Please see the response inline. I removed all the items that I have no further comments on.
On 03/25/10 11:44, Joseph J VLcek wrote: > Hi Karen, > > The updated webrev can be found here: > http://cr.opensolaris.org/~joev/bug15163_r2/ > > My comments are below. > > Thank you for the review. > > Joe > > > On 03/12/10 07:41 PM, Karen Tung wrote: > > > >> >> - line 244-303: The "invalid_argument_found" variable is defined. I >> don't think >> it is necessary. Since any invalid argument will cause the program to >> exit. >> Why not exit immediately? Why waste time to do other computations? > > This method will prevent the user from running multiple times to > identify all invalid arguments. For example, in the worse case, if all > 8 required and optional arguments are bad, this method prevents the > user from running once, being notified the 1st argument is bad, > rerunning again to be notified the next argument is bad... and so forth. > > This method notifies the user of all invalid arguments in a single run. My opinion actually differs from what you have. If we encounter the first invalid argument, we immediately exit and print the error, people be able to very easily see what's the problem, because that's the only problem that's reported. On the other hand, if we check everything else, the output reporting the errors will become long, and it will distract people from finding what actually is wrong. In addition, people sometimes just missed specifying one of the arguments which caused the whole list of arguments to be shifted. So, all the follow on "errors" might not be errors at all, and checking all of them is useless. > > >> >> - line 251-267: I think all these code is not necessary because those >> values >> are not used for installation at all. > > > Even though this script does not use them today if any of them are > invalid it could indicate a seriously wrong environment. Also if a > future code change requires more of these arguments they are already > checked. > That's true. All other finalizer scripts only access and validate the arguments it uses. If other variables need to be used in the future, they can be added. The start of all the finalizer script checks to make sure 5 arguments are passed. I believe that's enough to validate that we are running in a sane environment. Thanks, --Karen
