Brussels team,
Here are my belated comments. Overall a great addition to the framework.
General:
* I'm sure you're aware, but there are a couple of files with empty
deltas, or seemingly unecessary changes. I'll mention them just in
case:
usr/src/cmd/dladm/Makefile
usr/src/uts/common/io/ipw/ipw2100.c
usr/src/uts/common/io/strplumb.c
usr/src/cmd/dladm/dladm.c
* General: There are a lot of new inter-related data structures and
functions. It would be nice to have a single block comment explaining
the design so that someone writing a new show-* command can easily do
so.
It would also help to organize the top portion of dladm.c so that data
structures aren't so spread out. For example, place all of the
print_fields_t arrays in one place, etc... Another way to organize
things would be to put all of the "ether" related arrays and
structures together (and do the same for other show-* objects) so that
one can easily see how they're related and how to create new show-*
subcommands.
* General: Is there a difference between a "field" (i.e.,
"ether_fields"), an "arg" (i.e., "link_args_t"), and an "attr" (i.e.,
"link_attr"? These terms seem to be used interchangealy here and
there throughout the code, and it hurts the readability of the code.
Please use a consistent naming scheme for data structures and
functions.
* 75: What is <name>? Is it "pf_name", or "pf_header"?
* 87: I don't understand the need for this union, nor what pf_mask is.
It doesn't appear to be a mask at all, since it's used in a switch
statement at 3494.
* 88: On a related note, what is the need to store an offset here rather
than a pointer to an actual buffer? I can see this being much simpler
if you define a single global wad of output buffers like:
struct {
col_link_name[BLABLA];
col_link_class[BLABLA];
/* ... */
col_vlan_vid[BLABLA];
/* ... */
} output_columns;
typedef struct print_column_s {
const char *pc_name;
char *pc_buffer;
/* ... */
} print_column_t;
static print_column_t show_link_columns[] = {
{ "link", output_columns.col_link_name, /* ... */ },
{ "class", output_columns.col_link_class, /* ... */ },
/* ... */
};
* 137: Why glom the show-{link,phys,vlan} buffers together, but not the
buffers for other show-* commands? This seems somewhat random.
* 150: The members of this structure aren't arguments (they're output
buffers, right?), so I don't think link_args_t is an appropriate type
name.
* 365,369: Inconsistent use of "-" and "_". Is there a rationale?
* 399: This looks a lot like dev_fields. Please expand the comment to
explain what its for.
* 424: There's one too many spaces at the end of this string.
* 444,449: Missing -o option for these subcommands.
* 1295-1311: This array is stranded here; the other similar arrays are
at the beginning of the file. Please organize these things so that
they're all in one place.
* 1371: Why the "void *"? It would be more straightforward to pass in a
pointer to a link_args_t as the last argument, no?
* 1639,1781,etc.: "j" is not an intelligible variable name. Same
comment for a bunch of other functions in this file.
* 1662: Can this code be less confusing please? At a minimum, it needs
comments. You're using pointer arithmetic to see which offset into
the array you're at to find out which field you're printing. This is
practically unmaintainable. At some point in the future, someone will
make a change to a print_field_t array somewhere and all of this will
fall appart unintentionally.
* 1664,1668,etc.: Where do these constants come from? This is very
fragile. Please replace these with a proper enum datatype, and put
that in print_field_t. You'll then be able to un-obfuscate all of the
pointer arithmetic related to my previous comment.
* 1671: There are dozens of individual "--" strings everywhere. Let's
just have a global "const char" and use that instead.
* 1805,1810,etc.: Same comment as above, please replace the use of
seemingly arbitrary numbers related to array offsets with proper enums
or manifest constants. This comment goes for the other functions in
this file with the same problem.
* 3577: This shouldn't be const (and you'll be able to lose the cast at
3593.)
* 4517: What are the error semantics of DLADM_STATUS_BUSY? It looks
like if DLADM_STATUS_BUSY is returned, we silently don't do anything
and pretend that the command succeeded.
* 5241: die() exits; there's no need for this exit() call.
* 5269: media, class, and flags are never used. You can pass in NULL
arguments to dladm_datalink_id2info() instead of passing in references
you'll never use. There are other calls to dladm_datalink_id2info()
like this where some of the arguments aren't used that you didn't
introduce, and you could fix those while you're at it if you want.
* 5687: flags is never used.
usr/src/lib/libdladm/common/libdllink.h
* General: You added these prototypes to libdllink.h, but the functions
are defined in libdladm.c. I'd move them to libdllink.c along with
the other functions declared in libdllink.h.
* 153-154: These lines are unnecessarily split.
usr/src/lib/libdladm/common/linkprop.c
* 218-241: Why are these needed? Are there actual alignment issues, or
are these used to quiet lint? Could memcpy() not be used instead?
* 223: Why htonl()? Since we're dealing with values that don't show up
on the wire, intuitively, we should be dealing with values in
host-byte-order. Am I missing something?
* 584: The semantics should be that if DLADM_OPT_ACTIVE isn't set, then
the caller doesn't want the changes applied to the running kernel.
While dladm currently doesn't have a command-line flag to do
persistent-only operations, it doesn't seem too hard to implement that
in the API.
* 595: If you got DLADM_STATUS_BUSY at 584, the caller will never know,
and you might return DLADM_STATU_OK, thus pretending that the property
was successfully set when it wasn't. Honestly, I don't understand why
DLADM_STATUS_BUSY isn't a regular error like any other error.
* 1577: I see this same arithmetic copied-and-pasted everywhere below
here. This smells like something that needs to be simplified.
* 1606: There is no need to map the linkid to a link name. Your ioctl
just maps the link name back to a linkid in the kernel. It would be
much simpler to just pass down the linkid. As you have things now,
there are three mappings:
1. dladm is passed a link name and maps that to a linkid to call
dladm_set_linkprop().
2. dladm_set_linkprop() ends up in dld_buf_init_common(), which maps
that back to a link name so that it can call your DLD ioctl.
3. The ioctl maps that back to a linkid so that it can look up the
link in the dls hash table.
Only (1) is needed.
* 1639: Need a space or two between functions.
* 1666: Why do you need this? The driver will be able to tell you
whether the speed passed in was valid for that device, right?
* 1711: I fundamentally disagree with the idea that libdladm somehow
knows something about acceptable link MTU values. Each driver must
check to make sure that the given MTU is acceptable anyway, so there's
no need for libdladm to do anything. Besides, references to Ethernet
and IP are out of place, as you might be dealing with a datalink that
has nothing to do with either.
* 1927: I don't see the need for GETINT64() here. Why not a simple
memcpy()?
* 2031: There are dozens of places in this file where we do:
if ((fd = open(DLD_CONTROL_DEV, ...)) M 0) {
bla bla
}
i_dladm_ioctl(...);
A helper routine that does this would help reduce the duplication
(i_dld_ioctl, for example).
usr/src/uts/common/io/dls/dls_vlan.c
* 967,982: The model for the dls client APIs is that they take a
datalink_id_t to identify which datalink to operate on (the
i_dls_vlan_hash is by datalink_id_t.) Passing in a link name means
that the dls module has to map this to a datalink_id_t before doing
anything, which means a round trip up to user-space. If you follow
the trail in the code you have, which does a dls_vlan_hold_by_name(),
eventually, you end up in dls_mgmt_get_linkid(), which does the upcall
to the dlmgmtd daemon.
Having these functions take a linkid would allow them to do a simple
dls_vlan_hold() instead of dls_vlan_hold_by_name(). You'd then do the
link-name to datalink_id_t translation in user-space prior to calling
the libdladm API.
Such a change would ripple up to the ioctl handling in dld, obviously.
usr/src/uts/common/io/mac/mac.c
* 2971,2985: cstyle: curly brackets needed.
* 3000: The mac_info_t must be comprised of immutable values (see the
comment in mac.h.) The idea is that mac_info_t is copiedThe min and
max sdu need to be moved out of mac_info_t, and directly into
mac_impl_t.
-Seb
