Clay, installadm.c:
Thanks for adding the block comments. Just a couple of nits... 606 - you should combine 606 and 607 so that the name of the function comes first. It looks a little odd the way you have it. Do something like this: /* * do_list: List A/I services or print service manifests and criteria * * Parse the command line for service name; ..... * ...... */ 870 - Same as 606 964 - Ditto. Everything else looks ok. Also, note that I just pushed some changes to installadm.c and installadm.h. Sue Clay Baenziger wrote: > Hi Sue, > My apologies about these spacing issues, I'm reviewing the > webrev's now to ensure they get (and stay!) fixed. Please see > http://cr.opensolaris.org/~clayb/bug4103/webrev3. > Thank you, > Clay > > On Tue, 21 Oct 2008, Susan Sohn wrote: > >> Clay, >> >> >> Clay Baenziger wrote: >> > 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")? >> >> My reasoning is that if you have a message that is printed only in that >> place in >> the code, then you will know that the subcommand failed. The user doesn't >> need >> to know the specifics, only that they can't do the list the way they >> wanted. But >> I can live with what you have now. > > Okay, I like version as it's descriptive enough for them to understand the > problem is with installadm starting/running the subcommand not the > subcommand itself having a problem (i.e. with the A/I service data, etc.). > >> >> 855 and 863 - same as 611 >> > >> > Fixed >> >> This is still wrong. >> >> >> 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 >> >> Yes, now it makes sense. > > Cool, my apologies about the first version that was pretty vague. > >> >> 927 - similar comment to 692 above. >> > >> > I fixed this as 692 >> >> See my response above. >> >> >> 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? >> >> Yes, I meant that it should return one or the other, not just return. > > Cool, I think I now will return something in all cases; if you see > otherwise still let me know. > >> >> 938 and 947 - line up >> > >> > Fixed >> >> No, it doesn't look any different. >> >> >> 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. >> >> Then return INSTALLADM_FAILURE. I thought it was a success case since the >> failure was handled above. The point was to return INSTALLADM_<something>, >> not >> just return. > > Cool :) > >> Thanks, >> Sue >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
