>
> On Mon, Jul 06, 2009 at 03:36:11PM -0700, Michael Hunter wrote:
> > 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/
> >
> > We request code review be finished by Monday July 20th.
> >
> > Michael Hunter
I reviewed the libnwam and netcfgd changes.. overall looks good- the
addition of these features is a big step forward for nwam! here are my
comments.
--Sowmini
---------------------------------------------------------------------------
General:
- what is the difference between the netadm and netcfg uids? The
Brussels II project is considering setting up /sbin/ipadm as owned
by some dladm-like uid, and we had assumed this would be netadm.
Should it be netcfg?
- why is netcfgd under $SRC/cmd/cmd-inet/lib/netcfgd? Seems like this is
parallel to dlmgmtd for layer 3, so to be symmetric
with dlmgtd, shouldn't it be under $SRC/cmd/netcfgd/*? at the very least,
it would make sense to have $SRC/cmd/cmd-inet/nwam/netcfgd and
$SRC/cmd/cmd-inet/nwam/nwamd ($SRC/cmd/cmd-inet/lib is misleading, since
I don't think these apps get installed in /lib), but I think the
cleaner solution is
$SRC/cmd/nwam/netcfgd
$SRC/cmd/nwam/nwamd
$SRC/cmd/nwam/common /* for common themes like daemonize() */
- it's hard to figure out which libnwam interfaces are "truly" exported,
and which ones are there because they are shared between some daemon
and libnwam. The nwam_backend_init is an example of the latter, which
ends up in mapfile-vers as globally exported just so that the daemon
can also use it. We need to find a better solution for the latter case
(see comments for libnwam.h)
-------------------------------------------------------------------------------
Reviewed with no comments:
usr/src/cmd/cmd-inet/lib/netcfgd/util.h
usr/src/lib/libnwam/amd64/Makefile
usr/src/lib/libnwam/i386/Makefile
usr/src/lib/libnwam/sparc/Makefile
usr/src/lib/libnwam/sparcv9/Makefile
usr/src/lib/libnwam/Makefile.com
usr/src/lib/libnwam/common/mapfile-vers (except as noted in comments for
libnwam.h)
usr/src/lib/libnwam/common/door.c (deleted file)
usr/src/lib/libnwam/common/libnwam_error.c
usr/src/lib/libnwam/common/libnwam_events.c
usr/src/lib/libnwam/common/libnwam_values.c
usr/src/lib/libnwam/common/libnwam_wlan.c
usr/src/cmd/cmd-inet/lib/Makefile
usr/src/lib/libnwam/README
-------------------------------------------------------------------------------
usr/src/cmd/cmd-inet/lib/netcfgd/Makefile
- dlmgmtd runs as OWNER dladm, group sys. What are the owner/group
of netcftd (I'd guess that the owner would be netcfg- but inside the
code it seems to use netadm, and invokes getpwam etc. - why the
divergence from dlmgmtd)?
- do we need to define ROOTCFGDIR/<configfile> permissions?
usr/src/cmd/cmd-inet/lib/netcfgd/logging.c
- see relevant comments for netcfgd/main.c
usr/src/cmd/cmd-inet/lib/netcfgd/main.c
- fg can be a local variable to main() - it is not used outside the main()
context.
- debug doesn't need to be extern global, it can be static to logging.c
but perhaps it is not needed at all? How is one supposed to set it?
(One suggestion, always have syslog support, but use SIGUSR1/SIGUSR2
handlers to make it more/less verbose by incr/decr debug)
- zonename does not need to be a global var: it's only usage is for
lookup_zonename, and there it is passed as an argument.
usr/src/lib/libinetcfg/Makefile
- not sure I understand why this is necessary? According to
$SRC/lib/README.Makefiles, after you define HDRS and HDRDIR, you one need
check: $(CHECKHDRS)
Could you explain why this is needed>
usr/src/lib/libinetcfg/common/inetcfg.c
- icfg_get_linkinfo is a dead function - can be removed
- Not your introduction, but icfg_get_flags(), icfg_get_metric(),
icfg_get_mtu(), icfg_get_index() should bzero the lifr
as a first step. Also, these are exported functions that could be
called with a NULL second argument, so they should verify that
the flags/metric/etc. is not null before trying to set them.
- line 1854: list == NULL, can happen if there are no links
available, so I think this should only return ICFG_NO_MEMORY
if lwp->lw_err = ENOMEM. also it may be that we ran out of
memory half way through the walk, so we may still need to free
memory. i.e., we shoudl goto done, and do
for (i = 0; i < lwp->lw_num; i++) {
free(..);
}
- line 1863: why only AF_INET?
- icfg_plumb(): does this need to verify zone_check_datalink()?
(ifconfig does this in inetplumb before calling ifplumb)
- Comments at line 2333-2334 are incorrect - after the fix for
CR 6243060, the kernel only allows modifications to IFF_IPv4, IFF_IPV6,
IFF_BROADCAST, IFF_XRESOLV. Thus you could skip lines 2339-2345, and
instead start with lifr_flags of 0, before doing lines 2348-2354.
- lines 2368-2374: call icfg_get_flags instead of inlining this code.
- is it necessary to expose errors like ICFG_NO_UNPLUMB_ARP/ICFG_NO_PLUMB_IP
etc? How is the caller supposed to make sense of these errors?
i.e., collapse lines 86-90 to two errors, ICFG_IF_CREATE_ERR and
ICFG_IF_DESTROY_ERR (or some such pair of error codes)
- Question: it looks like we need separate icfg_handle structures for
IPv4 and IPv6 interfaces with nwamd_plumb_unplumb_interface being passed
an af to plumb. How is the af determined (seems like the
nwamd_ncu_state_machine figures this out. Also, I would have expected
the code to do something like
icfg_open()
plumb interface
add addresses
... other stuff with the interface..
and maybe only do the icfg_close when the interface is unplumbed
(but even that would result in needless creation of the ifh_sock), but
I see an icfg_open/icfg_close pair for each operation like icfg_plumb,
icfg_add_ipaddr etc. which would give rise to 6745288-like problems?
Ideally we should have a single handle for ipv4 and ipv6 (with
ifh_sock4, ifh_sock6), and then use the address itself to figure out
which af was targetted (as needed). If that's not possible (because
there are a lot of functions like icfg_set_mtu/icfg_set_metric which,
though they are dead code with questionable semantics, are not
getting cleaned up by this proposal) we should be restricting nwam
usage of icfg_open to retain handle(s) for as long as possible, instead
of doing an open/close per operation.
- icfg_unplumb: if called with an ipmp interface, do we want to bail?
(you would notice IFF_IPMP at line 2541)
- If the PUNLINK of the arp stream fails at
2570 } else {
2571 ret = ICFG_NO_UNPLUMB_ARP;
2572 goto done;
2573 }
the code bails out and doesn't try to unplumb the IP stream, whereas
onnv seems to- was this difference intentional?
usr/src/lib/libinetcfg/common/mapfile-vers
- see coments for inetcfg.h - looks like only icfg_plumb, icfg_remove_ipaddr
amd icfg_unplumb need to be added.
usr/src/lib/libinetcfg/common/inetcfg.h
- icfg_is_logical is not exported, so it can be made static to inetcfg.c
Similarly icfg_is_loopback, icfg_if_protocol.
usr/src/lib/libinetcfg/common/llib-linetcfg
- no changes to this file?
usr/src/lib/libnwam/Makefile
- ok except for similar question about the check target as for inetcfg/Makefile
usr/src/lib/libnwam/common/libnwam_audit.c
- line 69: do you need to adt_free_event() and adt_end_session()?
- line 49: do you need to adt_end_session()?
usr/src/lib/libnwam/common/libnwam_backend.c
- how does nwam_backend_door_server work (doesn't it end up doing
door_return 2 times?): on receiving some NWAM_BACKEND*REQ, it sets up
cmd = NWAM_BACKEND_*RES, and then goes to nwam_create_backend_door_arg
which will do a door_return at line 181. Then nwam_backend_door_server()
will again do a door_return at line 325, right?
- nwam_backend_init(): lines 349-360. dlmgmtd had some issues with the
filesystem not being writable early in boot. How does nwamd work
around that problem?
- if fattach() fails at line 382, we should door_revoke() the DOOR_FILE.
- nwam_backend_exit() is something that is only used by netcfgd,
and it registers an atexit() callback, so it doesn't look like it belongs
in a general "libnwam/common" file.. this should be made local to
netcfgd/*.c itself.
- should this be 0755
358 (chmod(NWAM_DOOR_DIR, 07555) < 0 ||
- make_door_call() is a rather generic function name (and libscf also
has this). Suggest nwam_make_door_call() or nwam_door_call() instead.
usr/src/lib/libnwam/common/libnwam_impl.h
- please use unique prefixes for fields to aid cscope. e.g, use nwh_ as the
prefix for fields in struct nwam_handle, nwv_ for struct nwam_value fields.
Just the simple "name" field is currently used in 5 different structures
even withing libnwam_impl.h itself.
- nit:
56 struct nwam_value
57 {
should be collapsed to one line:
56 struct nwam_value {
- please add some comments explaining what the various fields in
nwam_backend_door_arg are (e.g., arg_parent, arg_object etc.) and
what the layout of the data passed between client and server looks like
- nit: s/ist/list in
370 * A ist of all object files is returned. For ENMs
usr/src/lib/libnwam/common/libnwam_files.c
- value_add_escapes(): instead of repeatedly doing malloc/free around this
function, the caller can pass in the output array with its size. another
alternative is for value_add_escapes() to create out by doing a strdup(in)
similarly for value_remove_escapes()
usr/src/lib/libnwam/common/libnwam_enm.c
- nwam_enm_handle_create: is this function live? it's not exported via
mapfile-vers or libnwam.h, and there are no internal calls to it?
Functions that seem to need it (e.g., nwam_enm_read) seem to eventually
create the handle by calling nwam_handle_create itself.
If it's not needed, nwam_enm_handle_create should be removed.
- nwam_enm_set_name(), nwam_enm_get_prop_type() also appear dead.
- How does nwam_enm_handle_create verify its args (e.g., that name is null
terminated, non-null etc)? Same question applies to nwam_enm_read()
which *is* exported, so could get anything for its args. Minimally it should
take a name_size argument in addition to the char *name. Sanity checking of
flags wold also be a good thing.
usr/src/lib/libnwam/common/libnwam_known_wlan.c
- nwam_known_wlan_handle_create()
usr/src/lib/libnwam/common/libnwam_loc.c
- nwam_loc_handle_create(): is this function live? not exported, and no
calls to it? See comments for libnwam_enm.c
- nwam_loc_validate(): where is char *name freed?
- lines 325-331: please move these definitions to the start of the file-
they seem to be sprouting in randomp laces.
usr/src/lib/libnwam/common/libnwam_ncp.c
- nwam_ncp_get_read_only() leaks char *name
usr/src/lib/libnwam/common/libnwam_object.c
- please add comments before each function, explaining input/output
and expected returns from these functions. The "parent" term, for example,
is not inutitive- it seems like this is the name of the configuration
file/db from which the backend is supposed to read/write- is that correct?
If yes, it would be more obvious to rename "parent" to "dbname" or
"conf_file". Using the term "parent" for this makes it sound like
there would be a related "child", which is not the case here (for file-store).
- lines 1445-1460 please move these definitions to the start of the file
- lines 1768-1769: why have NWAM_IP_VERSION_IPV4/NWAM_IP_VERSION_IPV6
when IPV4_VERSION and IPV6_VERSION are available?
- doesn't valid_link_mtu have to check if the mtu is within the acceptable
range for the link? More generally, I would think that the signature
of the prop_validate function would need more args to verify against
libdladm, etc.?
usr/src/lib/libnwam/common/libnwam.h
- what is NWAM_FLAG_GLOBAL_MASK used for? Should this just be "-1"?
- why the extra level of indirect for NWAM_WALK_FILTER_MASK? Can't we just
have
#define NWAM_WALK_FILTER_MASK (0xFFFFFFFFULL << 32)
- use parentheses around NWAM_FLAG_NCU_TYPE_LINK, NWAM_FLAG_NCU_TYPE_INTERFACE
But a bigger question is - why are these being shifted out by 32 (i.e.,
what's in the low order bits, and why is it so complex?)
- need parentheses around NWAM_FLAG_NCU_CLASS_* definitions. But the
class definitions are confusing: on the one hand PHY and IPTUN
seem to be a subset of datalink_class_t (and therefore mutually exclusive),
but then there's CLASS_IP (which applies to any datalink_class_t).
How are distinctions drawn here? At the very least, some comments would
be needed to explain when/how these should be specified.
- it's unfortunate that nwam_backend_init and nwam_backend_fini are exposed
as global interfaces to any application that links libnwam, when
really, all that's needed is to share them between libnwam and
netcfgd. I'm not sure if SUNWprivate can be used here (?), but one other
option is to pull the related functions into a file (seems to be
libnwam_backend.c in this case) and link the file separately into
libnwam and netcfgd (e.g., see the usage of $SRC/common/net/patricia/radix.c
in $SRC/cmd/ipf/tools/Makefile.tools where the same code is shared
between the kernel ip module and ipfilter . This will allow the functions
themselves to be declared static, but bug fixes in the code will be
automatically picked up by both users, without exposing the interface
to other library callers.
- nwam_addrsrc_t: fyi, Brussels is introducing similar definitions.
Perhaps it would be useful to extract this to a common file that can
be shared by both libraries (and without any NWAM_* prefix)?
- NWAM_STATE*STRING definitions do not have to be exposed via
libnwam.h: should be mapped using a {code, string} array line nwam_errors
that's used by nwam_state_to_string()
see related comments about libnwam_util.c. The same thing applies to
nwam_aux_state_t, NWAM_AUX_STATE*STRING,
nwam_activation_mode_t, NWAM_ACTIVATION_MODE_*STRING,
nwam_condition_t, NWAM_CONDITION*STRING,
nwam_condition_object_type_t, NWAM_CONDITION_OBJECT_TYPE*STRING
nwam_configsrc_t, NWAM_CONFIGSRC_*STRING
nwam_nameservices_t, NWAM_LOC_PROP*
nwam_ncu_type_t, NWAM_NCU_TYPE*STRING
nwam_ncu_class_t, NWAM_NCU_CLASS_*STRING
nwam_iptun_type_t, NWAM_IPTUN_TYPE*STRING
nwam_addrsrc_t, NWAM_ADDRSRC*STRING,
nwam_priority_mode_t, NWAM_PRIORITY_MODE*STRING
- nwam_iptun_type_t can just use clearviews' iptun_type_t?
usr/src/lib/libnwam/common/libnwam_util.c
- the nwam*_to_string functions should use {code, string} arrays like
nwam_errors[]. In fact, there can be one nwam_code_to_string_internal()
that takes the {code, string} array and code as args, and returns the
const char * as the result. Then the nwam_code_to_string_internal()
(which would be a generalized version of nwam_strerror()) could be shared
by nwam_object_type_to_string etc. and this would also consolidate
the dgettext calls.
_______________________________________________
networking-discuss mailing list
[email protected]
