> Date: Mon, 06 Jul 2009 15:36:11 -0700
> From: Michael Hunter <[email protected]>
> To: [email protected]
> Subject: [nwam-dev] NWAM Phase 1 Code Review request
>
>
> This is a request for people to review the NWAM Phase 1 code.
>
> The code can be found at http://cr.opensolaris.org/~mph/nwam1_cr/
>
> For those inside sun there is a cscope database
> in /net/coupe.eng/builds/mph/nwam1_cr/usr/src (sorry to those external
> to SWAN).
>
> Documentation can both be found in the source tree and on the project
> web page at http://opensolaris.org/os/project/nwam/
Michael,
Sorry for the delay; my feedback on nwamcfg.c follows. I do not have time
to review all of libnwam, but if there are specific parts the team would
like me to look at, please let me know.
* General: many of the same comments I made for nwamadm apply
to this file -- e.g., zerr(), die_nwamerr(), CMD_* vs command
tables, etc. In the interest of brevity, I have not repeated
previous comments that apply directly to nwamcfg (e.g.,
problematic use of basename()).
* General: a lot of very repetitious code -- I have enumerated a
number of instances below. FWIW, a lot of these issues are due
to the libnwam API itself -- e.g., sets of parallel functions
that differ only in the handle they take and their name. It
seems like an OO design would've simplified the code
considerably -- e.g., each object could've had an ops vector
associated with it that the various callers could use without
repeatedly concerning themselves with the type of the object.
* General: because of the above, I have mostly focused on smaller
opportunities for code cleanup. That is, I find the libnwam API
problematic because it leads to rampant duplication and code
sprawl for consumers like nwamcfg, but aside from a couple of
examples to explain what we could be done "someday" I have
avoided commenting on it unless I have a specific and
straightforward suggestion for streamlining the code.
* General: assorted uses of printf() without localization.
(e.g., 918-920, 1984, 3060, 4331, 4335, ...)
* General: assorted spurious ARGSUSED directives -- e.g.: 3051,
3076, 3101, ...
* General: the various got_*_handle globals seem unnecessary;
shouldn't we be able to just check if the handles are NULL?
* General: numerous calls to check_scope() could be more simply
handled by the function dispatcher, rather than repeated in each
*_func().
* General: several blocks of code have something like:
if (got_enm_handle) {
prop_type = pt_to_prop_type(pt_type, NWAM_OBJECT_TYPE_ENM);
if (prop_type == NULL)
goto prop_error;
/* check if property can be set */
if (is_prop_read_only(prop_type, NWAM_OBJECT_TYPE_ENM))
goto read_only;
if (!show_prop_test(prop_type, enm_prop_display_entry_table,
NWAM_OBJECT_TYPE_ENM))
goto skip_prop;
/* ... */
} else if (got_wlan_handle) {
prop_type = pt_to_prop_type(pt_type,
NWAM_OBJECT_TYPE_KNOWN_WLAN);
if (prop_type == NULL)
goto prop_error;
/* check if property can be set */
if (is_prop_read_only(prop_type, NWAM_OBJECT_TYPE_KNOWN_WLAN))
goto read_only;
if (!show_prop_test(prop_type, wlan_prop_display_entry_table,
NWAM_OBJECT_TYPE_KNOWN_WLAN))
goto skip_prop;
/* ... */
} else if (got_ncu_handle) {
/* ... */
This code could be simplified significantly if there was a
function that consulted the various handles to determine which
one was live and returned the appropriate object type. Then
you could do something like:
handle_type = get_handle_type();
if ((prop_type = pt_to_prop_type(pt_type, handle_type)) == NULL)
goto prop_error;
if (is_prop_read_only(prop_type, handle_type))
goto read_only;
if (!show_prop(prop_type, prop_table[handle_type], handle_type))
goto skip_prop;
... and only switch on the handle_type once you need to get
into logic that cannot easily be parameterized.
* 32: I don't see any block comments near the end of
nwamcfg_grammar.y.
* 163, 167, 187, 189, 193, 214: What is the significance of these
comments?
* 219, 228, 236: Why are these arrays NULL-terminated?
* 240, 286, 304, 359: Why are these all different structures?
They all have identical contents and field names. These should
be folded into a common nwamcfg_prop_table_entry structure and
all members should have a consistent prefix for easy cscoping.
* 399: Seems like `output_file' could be local to export_func()
and passed to the various walker callbacks via the walker
callback argument.
* 405: Seems like quoted_strings could just be an argument to
output_prop_val().
* 447, 451, 455: Indeed, errors need to be reported.
* 483: Doesn't seem to have anything in particular to do with
strings -- would expect `array_free(void **array, int nelem)'.
* 962-986: Assumes English locale.
* 1043: prompt[] only accounts for MAX_NWAM_TYPE_LEN once, but
there can be two types.
* 1084: Why is it safe to ignore the return value from
string_to_yyin()?
* 1218-1265: Should be condensed to a single is_prop_listprop()
function that takes the table pointer. (That said, I would've
expected this attribute to be part of the object itself and
thus tracked by libnwam rather than nwamcfg.)
* 1355-1378: Mapping between NWAM classes and NWAM object types
seems more suited to libnwam.
* 1390-1392: Under what situations do callers pass in NULL?
* 1396, 1398: Crashes if strdup() fails.
* 1418-1659: Most error paths in this function leak `newname'.
* 1459-1519: Handling of `ret' can be centralized (cmd_res1_type
and cmd_res2_type can be converted to strings if precise error
messages are desired).
* 1525-1625: Lots of repetitious parallel logic here. This is
exactly the kind of code that would be simple and clear had
libnwam associated an ops vector with its objects rather than
having a set of equivalent but distinctly-named methods for
every operation.
* 1654-1655, 1925-1926, 1977-1978, 4859-4860, 2140: Shouldn't
alloc_cmd() be able to guarantee this?
* 1664: nit: s/than/the.
* 1671-1738: Logic to process the return codes should be shared
across these functions -- e.g.:
int
destroy_ncp_callback(nwam_ncp_handle_t ncp, void *arg)
{
return (destroy_ret(nwam_ncp_destroy(ncp,
NWAM_FLAG_DO_NOT_FREE));
}
* 1752-1764: What happens to nwamcfg when this to fails part-way
through?
* 1778-1783: Given the way this function is used, it would seem
simpler to change this be have_ncus() { return (1) } and have
the caller check if the NWAM_WALK_HALTED was returned to
determine whether or not the list was empty.
* 1896: If nwam_ncp_walk_ncus() fails, it appears we delete the
ncp regardless.
* 1935-1955: Duplicates logic in cancel_func().
* 1973-1982: Duplicates logic in end_func().
* 2019-2052: Should be backed by a common implementation that
takes a propname -- but moreover, I'd expect these utility
routines to be provided by libnwam.
* 2028, 2030, 2046, 2048: Returning -1 seems incorrect since it's
out-of-range for the type. Would seem simplest to add e.g.
NWAM_NCU_TYPE_UNKNOWN to the enums.
* 2143, 3577: nit: Superfluous `return'.
* 2201, 2221: Leaks `name'.
* 2223: nit: Superfluous blank.
* 2240-2344: This logic could be shortened considerably by having
the table pointers stored in an array that's keyed by the type
-- i.e.:
int
prop_to_pt(const char *nwam_prop, nwam_object_type_t type)
{
nwam_prop_table_t *table;
assert(type >= 0 && type < NWAM_OBJECT_TYPE_KNOWN_WLAN);
table = prop_tables[type];
for (i = 0; table.prop_type != NULL; i++) {
if (strcmp(nwam_prop, prop_table[i].prop_type) == 0)
return (prop_table[i].type);
}
return (-1);
}
* 2352: Why is that a reason for cutting and pasting this function?
* 2389-2399: Doesn't strtol() already provide this ability by
checking if isdigit(*endptr)?
* 2406: Why is `input_str' `const char *'? Doesn't seem like this
function treats it as const (as per the logic on 2432-2443).
* 2453-2467: val[] array is leaked.
* 2543-2544: How will this value be maintained? Seems like
nothing uses more than two values though.
* 2777-2780: `return (!prop_found)' seems simpler (if the concern
is clarity, giving the boolean another name (like `show_prop')
would help.
* 2843: If there are none then why do we check?
* 2923-3018: See previous comments regarding get_handle_type()
for how this could be streamlined.
* 3051-3151: Not to harp on this too much, but this is another
instance where one really pines for an ops vector -- e.g.:
int
list_obj_callback(nwam_obj_t *obj, void *arg,
const char *objname, const char *objplural)
{
char *name;
nwam_error_t ret;
boolean_t *list_msgp = arg;
if (*list_msgp) {
(void) printf("%s:\n", objplural);
*list_msgp = B_FALSE;
}
ret = (*obj->namefunc)(obj, &name);
if (ret != NWAM_SUCCESS) {
warn_nwerr("List error: cannot get %s", objname);
return (1);
}
(void) printf("\t%s\n", name);
free(name);
return (0);
}
... then e.g.:
int
list_enm_callback(nwam_enm_handle_t enm, void *arg)
{
return (list_obj_callback(enm, arg, "ENM", "ENMs"));
}
* 3181-3258: This is really hard to read. One minor improvement
would be to have the logic at 3193-3257 just build the string
arrays, and have a single loop iterate through them at the end
and centralize the fprintf() and NWAM_VALUE_DELIMITER_STR logic.
* 3322-3334: Needs to share a common implementation with
output_propname().
* 3322-3334: Shouldn't bisect the *_listprop() routines.
* 3431, ...: nit: Why is list_msg local to each `if' clause?
* 3556-3558: Why is this needed?
* 3564: Shouldn't list_ncu_callback() already print this?
* 3573, 4120: Superfluous check.
* 3581-3590, 3671-3681, 3717-3727, 3769-3779: Functions can easily
be consolidated into a single function that takes a type.
* 3830-3832: L1 exports would be cleaner to implement as an
export[] array indexed cmd_res1_type -- e.g. 3915-3937 would
essentially be:
if (cmd->cmd_res1_type == RT1_UNKNOWN) {
for (i = RT1_MIN; i < RT1_MAX; i++)
export_l1[i] = B_TRUE;
} else {
export_l1[cmd->cmd_res1_type] = B_TRUE;
}
* 3888: Short comment explaining the purpose of the
write_to_file check would be helpful.
* 4166-4191: See previous comments about get_handle_type().
* 4244-4275: See previous comments about get_handle_type().
* 4296: How can we get here with prop_type != NULL?
* 4343-4611: Significant amount of duplicate code here that
should be refactored -- e.g., lines 4356-4362, 4426-4432,
4493-4500, 4568-4574 all identical (and seems like functionality
that could partially be built into output_prop_val()), and much
of the user input processing could be factored out to a common
function and parameterize based on the type.
* 4338-4340: Seems more complex to do it this way than to
just have 4332 and 4335 print the closing brace.
* 4621-462: nits: s/new/newly/, s/value/a value/,
s/current/the current/
* 4659-4770: Duplicate walking logic should be factored out.
* 4830: Indeed, an error here would be good.
* 4839-4852: Essentially duplicates the logic from 1084-1095.
* 4899-4908: What would go wrong if we attempted to use a
file that wasn't a "regular file"?
* 4854-4865: Duplicates logic in end_func().
* 4918-4925: Duplicates logic in destroy_func().
* 4959: Shouldn't cmd_file_mode already be B_FALSE?
--
meem
_______________________________________________
networking-discuss mailing list
[email protected]
