On Fri, Oct 12, 2007 at 11:00:19AM -0700, Zhenghui.Xie at sun.com wrote:
> I have an updated webrev with several cleanup changes.
>
> http://cr.opensolaris.org/~xzh/cli-glue-cleanup/
Makefile
nwamcfg_lex.l
-------------
Reviewed, no comments
nwamcfg.c
---------
73-75, 82-83 These use 'resource', 'resource1' and 'resource2'; it's not
clear to me what the distinction is or when you would use one
over another. Was it intentional that when <name> was added
to the create string, 'resource1' also became 'resource'?
174 "condition" isn't placed here in the same order it appears in
the PT_ #defs in nwamcfg.h.
long_help() Need to add new commands here?
556, 568, 573 Since we have the "got_foo_handle" booleans, I think it would
be better to use those for the test. If those are used every-
where, you wouldn't need to explicitly set the handle pointers
to NULL, either. Alternatively, get rid of the booleans and
always use the NULL check. Looking at the rest of the code, I
think the former choice (use the booleans for tests and don't
bother setting the pointers to NULL) is better.
580-597 I thought conditions were just stored as a string now; is this
function used/needed?
600-623 Should wrap this function in an 'if (debug)' or something like
that, since it's called at the beginning of most command funcs.
655, 663 This test doesn't seem to match the comment ("not allowed at
interactive mode"; the test is for "not interactive AND not
cmd file mode"). Also, cmd_file_mode should probably be made a
global if it's going to be used in places like this (currently
it's a local var in main()).
686-688 env_h is leaked in this case; simply removing the return on
line 688 should take care of that.
710 Do we really want to free the ncp handle in this case?
799, 928 I agree that these are not needed for the deleting/inserting
of sets of svc entries in an env. However, the man page also
mentions that they are used to delete/insert NCUs from/into
an NCP. At this point, that functionality is covered by the
destroy/create subcommands, right? Assuming that's the case,
I think the delete/insert subcommands are not needed at all.
Be sure to update the man page accordingly.
809 Why isn't this allowed in interactive mode?
809, 818 Comment/test mismatch.
823 This comment is (I think) in reference to the absence of
something--the 'need_to_commit = B_TRUE' line; but it doesn't
really say anything other than "yes, it's intentional that
line isn't here." I think it's more confusing than helpful,
really.
889 I don't see time_to_exit being used anywhere; how does this
function work?
966 I assume cmd_res1_type is guaranteed to be RT1_NCP if
cmd_res2_type is RT2_NCU?
989 Nit: the variable name ncu_type is a bit deceptive. I think
ncu_prop might be a little more clear? Ditto for enm_type in
the corresponding enm function (line 1049).
1033 Those were removed because they apply to aggregations, vlans,
or ipmp groups, which won't be supported in phase 1.
1110 This should be NWAM_ANY_ADDR_TYPE.
1111, 1113 These should be grouped.
1151-1158 If the first strtok() returns NULL, you exit this block with
n == 1 but val == NULL, which will cause problems later when
you try to access val[i] in the loop that starts on line 1160.
1152, 1161 Both of these lines assume that an array of pointers exists,
but both variables were declared simply as NULL pointers.
You'll need to alloc arrays before alloc'ing the objects
pointed to by the array elements. For val, that's a little
messy, as you don't know how many elements you need; you'll
need to alloc/realloc as you go. Or do we have a max value?
For data, you could simply pull the calloc() out of the for
loop, and pass n rather than 1; but then you have an array of
nwam_data_ts, not pointers to nwam_data_ts, so you would also
have to change the references.
1180, 1190 Need to free val in these error cases.
1198 I think this needs to be atoll(), not atol() (a long may only
be 32 bits).
1265-1271 You could just list these cases first, set is_list_prop for
them, then fallthru for the rest of the processing.
1275, 1316 I think the third param should be num, right?
1289-1293 Actually, I think the activation type will simply be one of the
hardwired properties of the env, so once we have those and a
nwam_env_set_prop() function, that will be sufficient. We can
probably deal with the condition this way, as well, since that
ended up just being a string.
1347-1350 Like the activation type and condition, this should be one of
the hardwired properties. It will be read-only, of course.
1358, 1360, The handling of spacing seems odd; even with the extra spaces,
1380, 1393 things will only be aligned if the obj names are the same
length. You might want to consider tabs, or other ways of
formatting the output here. Possibly just a header for each
section, something like
ENVIRONMENTS
env1
env2
ENMS
enm1
NCUS
ncu1
ncu2
Also, for NCUs: probably want to include at least the type with
the name, since it will be possible to have a link NCU and an
ip NCU with the same name.
1362, 1382 This isn't a problem in your code; but we need to make sure the
library can handle a free from within the callback.
1387-1398 The format change of this function (vs. list_enm_callback()) is
a little confusing for the reader.
1334-1398 Some handle frees are missed: in all three functions, the handle
is not freed if the get_name() function fails; and it's not
freed at all in list_ncu_callback().
1467 In some cases, the hex value will be much more useful than the
decimal. Perhaps include that in parentheses?
1486, 1507 This is not needed; you do actually use all the args.
1570 A blank line here would be good.
1586-1590 As mentioned earlier, this should be part of the hardwired env
properties.
listprop_func() The foo_h global handles are used here without checking the
corresponding got_foo_handle booleans.
1653-1668 You can pull this section up one level of indentation.
1675-1725 These functions need to free the handles they're passed.
1767 Need to free ncp_h.
1779 s/enms/envs/
1804, 1826 Need to free enm_h.
2125 We want to exit after one_command_at_a_time(), right?
nwamcfg.h
---------
115 I think PT_ENM_START would be preferable; STR makes me think
it's a string of some sort.
nwamcfg_grammar.y
-----------------
67 As with the #define in nwamcfg.h, I think ENM_START would be
preferable to ENM_STR.
