Sebastien,
thanks much for the comments! I will go through them and get back to you..
--Sowmini
On (11/20/07 18:32), Sebastien Roy wrote:
>
> 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
> _______________________________________________
> brussels-dev mailing list
> brussels-dev at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/brussels-dev
