Hi Sundar,
Thank you for the review; comments inline; we're now on
http://cr.opensolaris.org/~clayb/bug4103/webrev1
Thank you,
Clay
On Mon, 20 Oct 2008, Sundar Yamunachari 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
> > _______________________________________________
> > caiman-discuss mailing list
> > caiman-discuss at opensolaris.org
> > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
> >
> Clay,
>
> Need block comments for do_list(), do_add() and do_remove()
I'm not sure what you mean by block comments, as I don't see block
comments for other do_* functions in the code. However, I have added some
explaination of what the subcommand's purpose is in case it's of use?
> There are alignment issues. Please run "hg pbchk' and fix all cstyle errors.
hg pbchk returns clean, so I'm not sure what you're seeing?
----------------------------------------------------------
[113] clayb at indiana-build: hg pbchk
CDDL block check:
Copyright check:
C style check:
Header format check:
Java style check:
File permission check:
Keywords check:
Comments check:
These comments are neither bug nor ARC case:
4103 Need installadm code to support recent Python manifest code
Checking for new tags:
Checking for multiple heads (or branches):
Checking for branch changes:
RTI check:
Checking for uncommitted changes:
Checking for merges:
> 617, 625: getopt() doesn't have "-c" option parsed. So case in in 625
> will never be executed.
I added the 'c' option to getopt.
> 637: print_criteria is always B_FALSE
Fixed from above
> 629, 639: The parameter "use" passed to the function doesn't have -c
> option in the use.
I removed this until after the November release, if you'd like me to
put the flags back for do_list (-c) and do_remove (-i) on 75 and 95 let
me know.
> 682, 908: Capture the return value from installadm_system() and return
> to the main() rather than existing from here (like lines 693-697). Also
> print a meaningful error message in the case of failure before returning
> like in line 695.
I've added an error which prints "Failure running subcommand." However,
the Python scripts themselves will printout an error, if one occurs which
they catch. So, the only thing this should be useful for, would be if we
couldn't execute Python. Otherwise, the script will handle errors.
> 677, 680, 907, 987, 992: Define a constant for /var/ai in installadm.h
> and use it these lines instead of using /var/ai directly.
Ah thank you, of course this is a good idea. Done.
> 940: Add a comment to indicate that "-i" is an internal option and not
> exposed right now.
I've done that for -i in remove and -c in list. (As, I believe you wished
to avoid exposing that feature until after November too?)
> - Sundar
>
>
