> 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,
Here are my comments on nwamadm, netcfgd and the DLPI logic in nwamd.
General:
* A number of functions are named without appropriate prefixes
(e.g. nwam_) in ways that encroach upon actively evolved public
namespaces. For instance, door_switch() and and dlpi_notify().
* A number of functions use chumminess with lint to make arguments
appear to be used (e.g., `foo = foo'). This is not a robust or
clear way to handle unused arguments; please use ARGSUSED or
the NOTE() facility to mark unused arguments.
* A number of places seem to check for pthread_mutex_lock()
failure. This is needless: there are no cases short of
programmer error that will lead to pthread_mutex_lock() failing
(for a normal pthread lock).
nwamadm/Makefile:
* Given the way gettext() is used in nwamadm, you need something
like XGETFLAGS += -a -x $(PROG).xcl and an nwamadm.xcl file.
* 34: Why is LFLAGS set in here?
* 36: -I. is wrong; local header files should be included with
`#include "local.h"'.
* 30: Needless; source is only built from one file.
* 42-44: Needless rules.
* 55: Why not use lint_PROG?
nwamadm.c:
* Throughout: the ofmt API needs to be used to print stuff out
rather than rolling your own printing logic. This will also
provide parsable mode and user-selectable fields for free
(which currently seem to be missing).
* Throughout: what's up with the name zerr()? Am I to assume it's
because this code started life as zoneadm.c? Seems like it
should be called die(), and seems like you could also use a
die_nwamerr() function that takes a nwamerr and does the
nwam_strerror() a la die_dlerr() in dladm.
* Throughout: seems a shame that the libnwam walker APIs require
a non-NULL cb_ret argument, as nothing here seems to want to
consult it. Why not allow it to be NULL?
* 50: Needless blank line.
* 58, 64-72: The use of both command numbers *and* an array of
command entries seems muddled. Seems like all the functionality
tied to command numbers could be folded into state tracked in
the command entries -- e.g., a cmd_help pointer-to-function (or
just a simple const char *), and a flags field that indicates
whether e.g. "-x" can be used or whether the type needs to be
determined.
* 60: Should be cmd_handler for consistency.
* 60: Should define a typedef for this -- e.g.:
typedef int (cmd_handle_func_t)(int, char **);
... then declare cmd_handler as a pointer to this, and the
declarations on 79-84 as instances of this type.
* 77: Seems like this FMRI should be in a library header.
* 151: Unclear why this is a global; seems like it should be
declared in interact_func() and passed to eventhandler().
* 167-277: I'd expect these helper routines to be in libnwam
rather than nwamadm. Some of them already appear to be there,
such as nwam_ncu_type_to_flag(). Why are they here?
* 179, 193, 215, 230, 246, 262, 275, 297: The default cases here
seem unhelpful. I'd expect strings of "?" and enumerations
along the lines of NWAM_OBJECT_TYPE_UNKNOWN.
* 209: Presumably this should be "ncu"?
* 323: Presumably this should be ""%s: <subcommand>".
* 368, 371: This seems like abuse of return values, as
NWAM_WALK_HALTED and NWAM_SUCCESS do not appear to be intended
for this use. Instead, it seems that the walker should return
0 or 1.
* 378: Why is it safe to ignore the return value from this
function? Is it that we assume `state' will only be modified
by this function call upon success? Seems a needless
assumption.
* 451: Would seem more natural to return `type' and remove the
`num' parameter in place of an NWAM_OBJECT_TYPE_UNKNOWN value.
* 514: This seems unnecessarily obscure; don't we know
cmd_to_str(CMD_LIST) == "list"?
* 549-558: Seems like this would just be:
if (cmd_num != CMD_LIST && type == NWAM_OBJECT_TYPE_NONE)
* 560: Needless cast.
* 653-684: This should be factored out into a funciton that takes
the type and then is merely called here for NWAM_NCU_TYPE_LINK
and NWAM_NCU_TYPE_INTERFACE. For bonus points, could take a
function pointer which would be either &nwam_ncu_enable or
&nwam_ncu_disable.
* 651, 690, 693: Should be a switch statement with NWAM_SUCCESS,
NWAM_ENTITY_MULTIPLE_VALUES and default.
* 705-715: Can factor this out to a function and reuse this code with
disable_func(). For I18N to work, you need to restructure the
messages -- e.g.: "cannot enable: <blah>" -- then you'll end up
parameterizing the subcommand name (which won't be translated).
* 719: By the time we get here isn't the enable complete? That
is, seems like this should be "Enabled", not "Enabling".
* 724: `if' block should be replaced with assert(ret !=
NWAM_SUCCESS)' and the code should be unindented.
* 785-868: See previous comments regarding enable_func() code.
* 871: Would seem preferable to return a error code indicating
whether the function succeeded rather than overloading
*_UNINITALIZED for the job.
* 872, 934: Everything that uses determine_object_state() immediately
then calls add_to_profile_entry(). Seems like it would be
simpler to have add_to_profile_entry() get the state itself
rather than taking `state' and `auxstate' arguments.
* 928: How do we get here? Seems like we should assert(0).
* 932: p_entries is a linked list, not an array.
* 956: p_last_entry is gross. Does the order of the list matter?
If not, why not prepend to the list? If so, would prefer to
make the list circular and use p_prev.
* 979, 1020, 1058, 1090: Needless cast.
* 986, 988: `name' appears to be leaked.
* 988: `val' appears to be leaked.
* 1108-1156: See previous comments about using ofmt. As an aside,
the field names are generally considered part of the command
itself and are not something we translate -- and this is
important when -o <field> is supported.
* 1165-1166: Needless "optimization".
* 1196-1199: No voodoo code, please.
* 1207, 1219, 1227, 1234: Needless cast.
* 1250, 1251: Useless comments.
* 1260-1475: This function is really hard to read -- and again,
ofmt should be used to print output.
* 1260-1475: Is this intended as a debugging aid or something that
end-users would actually use? If the latter, seems like some
I18N handling is missing in the various prompts. Also, how is
the end-user supposed to make sense of the stuff output from
this? Further, it seems odd to me that the interactive support
and event monitor have been glommed together into one facility.
* 1266: `action' should be declared properly as a `const char *'
and the bogus casts on line 1278, 1422, and 1449 should be
removed.
* 1338, 1346, 1388: This works without fflush(stdout)?
* 1338, 1346, 1388: Can the user ^C out? If so, how does that
work? If not, seems a usability problem.
* 1366, 1402: Why are errors going to stdout?
* 1378, 1388: Would expect a space after the ":".
* 1432: How do you know this is a sockaddr_in? Sure looks like
this code can be reached with a sockeddr_in6.
* 1474: Remove blank line.
* 1511: See previous comment about I18N with field names.
* 1515-1528: Odd code here seems to stem from mixing interaction
and event monitoring.
* 1543: strdup() can fail. This is the problem with using
basename(3C) rather than just doing the typical:
if ((execname = strrchr(argv[0], '/')) == NULL)
execname = argv[0];
else
execname++;
You can also remove the <libgen.h> #include and -lgen link.
* 1551-1554: This means one cannot observe the events at NWAM
startup with "interact" which seems unfortunate.
* 1531: So using the "interact" subcommand always exits with a
non-zero status?
* 1547, 1554, 1565: Would be clearer as EXIT_FAILURE.
* 1556: I suppose it doesn't matter much, but `state' is leaked.
netcfgd:
* Unclear why this consists of multiple source files; it seems
like everything could be consolidated into a single source file
without sacrificing maintainability.
netcfgd/Makefile:
* 29-30: Pulling in Makefile.lib seems wrong since that's for
building libraries. Just use $(ROOT)/lib.
* 35: Remove needless HEADERS definition.
* 61: What does this $(ROOTCMD) dependency do?
netcfgd/util.h:
* Given that this only has logging functions, seems odd it's named
util.h. (But then again, I can't figure out why this exists.)
* 33: Need a /* PRINTFLIKE2 */ here.
* 39: No externs, please.
netcfgd/logging.c:
* 28-29: Unclear what this comment has to do with anything; no
debug.c here.
* 42: s/The idea for having this function is so that you can
/This function allows you to/
* 43: s/Its/It's/
* 59: Why 256? Why not vasprintf()?
* 81-90: Could be replaced with (in util.h)
#define dprintf(...) nlog(LOG_DEBUG, __VA_ARGS__)
netcfgd/main.c:
* General: I can't figure out what the zonename code is for;
we never seem to use the `zonename' variable. My nits below
assume that somehow it's supposed to do something.
* 28: Comment should provide an overview of what this daemon does
on a technical level.
* 32, 34, 38, 39, 40, et al: Superfluous #includes
* 51, 52: No excuse for these being globals.
* 57-59: Unclear what this comment has to do with anything.
* 68: Where does this code lookup SMF properties?
* 70: Where does this code manage privileges?
* 76: Is there a reason this uses LOG_NDELAY?
* 79-113: Someone *really* needs to put this in a library :-(
Looks like this is cut line-for-line from Nevada nwamd/main.c
which in turn took it from pppoe/pppoed.c.
* 124: Prefer const.
* 126: No need to define `zoneid'; just inline the call to
getzoneid() int the getzonenamebyid() call.
* 132: Why is it correct to charge on with GLOBAL_ZONENAME in
this case?
* 141-142: Just curious: what exactly are we localizing? All of
the messages here go to syslog which is not localized.
* 145: Why is this LOG_INFO? Seems like LOG_DEBUG. Also, LOG_PID
was specified to openlog() so I'm not sure why this includes the
pid as well. Finally, the message should be generated in
start_logging(), not here.
* 149-155: Indenting here is one level off.
* 162: Comment states the obvious.
* 166-168: Unclear why we want to ignore USR1/USR2/INT.
* 170: Need to resolve this XXX for SIGTHAW.
dlpi_events.c:
* Throughout: libdlpi does not use `errno' for errors. All of the
dlpi-related log messages in this file are busted.
* 34: Unclear what net/route.h has to do with this file.
* 65: This `if' clause is needless: nwamd has requested to only
receive events of these two types.
* 67: Use of 'IFF_UP' to have some relationship to the link state
seems wrong. Seems like it would be simpler to change
link_state.link_state to link_state.link_up and just have it be
a boolean.
* 71: Shouldn't this message explain the administrative impact?
* 88-102: Unclear why this function is allocating a buffer to
receive into since it does nothing with this buffer -- and even
more unclear why this buffer is sized to DLPI_PHYSADDR_MAX.
Seems like this whole function should just be:
static void *
nwamd_dlpi_thread(void *arg)
{
int rc;
dlpi_handle_t *dh = arg;
for (;;) {
rc = dlpi_recv(*dh, NULL, NULL, NULL, NULL, -1, NULL);
if (rc != DLPI_SUCCESS) {
nlog(LOG_ERR, "nwam_dlpi_thread: dlpi_recv "
"failed: %s", dlpi_strerror(rc));
break;
}
}
return (NULL);
}
* 94: Bogus cast.
* 99: Unclear where the magic notion of three failures comes from;
one would think any unexpected failure would be worth stopping
for.
* 100: resumably *dh was meant?
* 105: This function would be much easier to read if a local
variable was declared for ncu->ncu_node.u_link.
* 123: Can this happen? If so, what prevents a situation where two
thread simultaneously pass this check and race to both call
pthread_create()?
* 135, 182: Comments only tell me what is clear already from the
code.
* 136-146: dlpi_bind() is no longer needed to use dlpi_enabnotify().
* 150: Sure seems like `strdup(name)' ends up getting leaked here.
Shouldn't this be NULL?
* 160: Presumably the intent is to record the error value returned
from pthread_create() in the nlog() message?
--
meem
_______________________________________________
networking-discuss mailing list
[email protected]
