On (11/20/07 18:32), Sebastien Roy wrote:
> Brussels team,
>
> Here are my belated comments. Overall a great addition to the framework.
Seb,
many thanks for the code review comments! Here are our responses:
> 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
Ok, will unedit this file.
> usr/src/uts/common/io/ipw/ipw2100.c
> usr/src/uts/common/io/strplumb.c
We're cleaning up these files as part of the general header file
resolution: there were circular dependancies between dld.h and mac.h
that were creating complications for some of the dld_ioc structures.
As part of straightening that out, we had to explicitly include
header files in the above 2 cases.
>
> 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.
Accept.
>
> * 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.
Accept. The names have all been normalized. all the print_field_t
instances are now called "<foo>_fields, and the corresponding buffers
are typedef'ed as <foo>_fields_buf_t
> * 75: What is <name>? Is it "pf_name", or "pf_header"?
pf_header; will clarify.
> * 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.
The "_mask" was inherited from the onnv code (wf_mask).
I agree that this is not always consistently used as a mask in onnv.
I'll rename this to pf_index, since that's more accurate.
> * 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, /* ... */ },
> /* ... */
> };
this would also work, but would need a lot of global output_columns
structures, whereas the current approach defines these on the stack.
> * 137: Why glom the show-{link,phys,vlan} buffers together, but not the
> buffers for other show-* commands? This seems somewhat random.
the original intention was to have separate buffers for each show
command, but during implementation, I noticed that the link/phys/vlan
commands have a lot of common fields. So, rather than duplicate
lots of buffer definitions, I tried to coalesce them here (seems
like this thinking was also followed for defining the longopts (e.g.,
show_lopts is used by show-link, show-aggr etc., lopts is shared
between aggr and vlan commands)
>
> * 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.
Ok- as mentioned before the names have been normalized to reflect
their purpose.
>
> * 365,369: Inconsistent use of "-" and "_". Is there a rationale?
speed-duplex is printing speed (and) duplex, whereas rem_fault
is printing the internal rem_fault param (*not* rem "and" fault)
> * 399: This looks a lot like dev_fields. Please expand the comment to
> explain what its for.
Accept (this is the structure for dladm show-dev -s)
>
> * 424: There's one too many spaces at the end of this string.
Accept.
>
> * 444,449: Missing -o option for these subcommands.
Accept.
>
> * 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.
Accept
> * 1371: Why the "void *"? It would be more straightforward to pass in a
> pointer to a link_args_t as the last argument, no?
Accept
> * 1639,1781,etc.: "j" is not an intelligible variable name. Same
> comment for a bunch of other functions in this file.
Accept
> * 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.
> * 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.
Accept (all the above are related) I could just store the index
explicitly in pf_index (the renamed "pf_mask").
> * 1671: There are dozens of individual "--" strings everywhere. Let's
> just have a global "const char" and use that instead.
Accept
> * 3577: This shouldn't be const (and you'll be able to lose the cast at
> 3593.)
Accept
dladm.c:
> * 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.
linkprop.c
> * 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.
Accept (both of above are related).
Regarding the semantics of DLADM_STATUS_BUSY:
The intention is that if the driver cannot set the property because it
is busy, then it will return the DLADM_STATUS_BUSY error code to
the caller, but the setting is still stored in the dladm persistent
repository (to be attempted at the next driver attach, via the
daemon). The processing of DLADM_STATUS_BUSY is more integral to the
persistence component, so it will be treated as any other error for the
framework.
And, while fixing this, I noticed that show-ether was missing in
dladm.xcl. I have added this.
> * 5241: die() exits; there's no need for this exit() call.
Ok, AI.
> * 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.
Accept, will fix as many as I find.
> * 5687: flags is never used.
Accept.
> 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.
Accept.
> * 153-154: These lines are unnecessarily split.
Accept.
>
> 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?
> * 1927: I don't see the need for GETINT64() here. Why not a simple
> memcpy()?
These were used to quiet lint errors- I was reluctant to suppress
the lint message by other means, since the layout of the structure
can change in the future, and we would have to deal with the alignment
problem at that time anyway. The htonl itself was used
to deal with byte-ordering differences (memcpy will run into
byte-ordering differences, and doing the byte-swap is messy for
int64's). I'll comment this to make it clearer.
>
> * 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.
need clarification: this would also be something that applies
to the parent clearview-uv gate, right? i.e., your comment is that
i_dladm_set_linkprop should only be invoked from dladm_set_linkprop
if DLADM_OPT_ACTIVE is set, correct?
> * 1577: I see this same arithmetic copied-and-pasted everywhere below
> here. This smells like something that needs to be simplified.
accept; will try to simplify this.
linkprop.c:
> * 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.
also relates to:
> 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.
Defer: this will imply some changes to the ARC'ed interfaces, and
is a little complex to define, without the supporting UV defintions.
We'll do this part as part of the Property Persistence component
of Brussels.
> * 1639: Need a space or two between functions.
Accept
>
> * 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?
The dld_binary_check verifies that the input is "0" or "1" (as
opposed to some random number like "7". This comment is related to:
>
> * 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.
Both of the above comments are related to the same question: should
libdladm do preliminary bound checks in user-space, or should
we rely on the driver to do all checks. One of the requirements
raised in early Brussels design was to try to do bound checking in
user-space as far as possible.
I do agree that checking the mtu itself can't be done reliably
for all drivers (as the comments in dld_defmtu_check indicate). So
I'll remove the ip-biased mtu check from defmtu_check.
>
>
> * 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).
A quick check reveals:
libdllink.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
libdllink.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
libdllink.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
libdllink.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
libdllink.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
libdllink.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
libdlvlan.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
libdlvlan.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
libdlvlan.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
libdlvlan.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0) {
linkprop.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
linkprop.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
linkprop.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0) {
linkprop.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0) {
linkprop.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0) {
linkprop.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0) {
secobj.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
secobj.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
secobj.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0)
secobj.c: if ((fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0) {
Yes, a helper function would be nice. I'll see what I can do.
>
>
> usr/src/uts/common/io/mac/mac.c
>
> * 2971,2985: cstyle: curly brackets needed.
Accept
> * 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.
This is actually something that I imported from clearview (which I just
noticed has been updated). BTW, we had discussed this earlier, and I
believe we concurred that we'd need a fast-track for this interface-
can the clearview-iptun help me with that part by fast-tracking this
into Nevada? When I tried updating as you suggest,
I found this change impacts several files that access mi_sdu_max
such as xnbo.c etc. so I'd like to defer this update for cv-iptun.
