Hi Sue,
Thank you for the comments; my responses are inline and will be
reflected in: http://cr.opensolaris.org/~clayb/bug4103/webrev2.
Thank you,
Clay
On Tue, 21 Oct 2008, Susan Sohn wrote:
> Clay Baenziger wrote:
> > Hello all (but especially Sue, and Sundar),
> > To support the manifest commands from last week here's the hooks
> > for installadm to actually call them. Please let me know if I've missed
> > any reasonable diligence, otherwise, let me know what code issues there
> > are.
> >
> > Code review: http://cr.opensolaris.org/~clayb/bug4103/webrev0
> >
> > Thank you,
> > Clay
>
> Hi Clay,
>
> I started to review webrev0, but then switched the line numbers and comments
> for
> webrev1, when I saw that mail. So this refers to:
>
> http://cr.opensolaris.org/~clayb/bug4103/webrev1
>
>
> 76 - why add a space in front of -n? It is inconsistent with the other command
> usage output
Oops, that was a typo from removing the -c option to list. Fixed
> 611 and 619 - you need to line up opt and ret with the other declarations
Fixed
> 692 - I'd suggest a more descriptive error message than Failure running
> subcommand. Something like Unable to list services, or something like that
> would
> be better.
I'm not sure how much more descriptive I can be but I want to ensure I
make it apparent that we're unable to even run the subcommand. How about
"Failure running subcommand %s" (i.e. "Failure running subcommand list")?
> 855 and 863 - same as 611
Fixed
> 883 - line exit up with line above
Fixed
> 923 - what does this comment mean? Please make clearer.
I cleaned all of this up a bit. Yes that comment was a bit of sleepy talk
> 927 - similar comment to 692 above.
I fixed this as 692
> 929 - return INSTALLADM_SUCCESS
I ensured do_add(), do_list() (both my code and the listing of
services), and do_remove() now return INSTALLADM_SUCCESS, in the case of
success. But failures should return INSTALLADM_FAILURE right?
> 938 and 947 - line up
Fixed
> 973 - same as 883
Fixed
> 1025 - return INSTALLADM_SUCCESS
I'm not sure why I'd want to return INSTALLADM_SUCCESS from this if as
it's a failure case.
> Sue
>
