Seb, Thanks for the code review; All the "Ok" responses below are fixed at
http://zhadum.east/export/ws/sowmini/nddcompatchild/webrev/index.html http://cr.opensolaris.org/~sowmini/sebrev/ --Sowmini > Here are my comments: > > usr/src/lib/libdladm/common/linkprop.c > -------------------------------------- > > * 210: This constant should be DLD_PROP_MTU to match the property > name. Ok. > * 700: This seems like a gratuitous change. I preferred the original > code, with a single return (status) statement at the end of the > function rather than return statements scattered within the switch > statement. Ok. > * 1017: No need for the brackets. Ok. > * 1943,1949,1968,1972: Please combine all of these return sites into a > single block at the end of the function which ensures that > everything that needs to be freed is freed, and that everything that > needs to be closed is closed. Otherwise, the code is likely to leak > (as in lines 1949 and 1968 leaking fd). Ok. > * 2027: I find it surprising that this is the applicable range for > Ethernet. For one, I'd expect to be able to lower the MTU of an > Ethernet link. Another issue is the limit of 9000. Is that a > universal hard limit for all Ethernet links? E.g., why shouldn't I > be allowed to write a DL_ETHER driver which I can play around with > MTUs higher than 9000 without having to recompile libdladm? The actual minimum size of an ethernet frame is 64 bytes. (Destination Address (6 bytes) + Source Address (6 bytes) + Frame Type (2 bytes) + Data (46 bytes) + CRC Checksum (4 bytes)). However, all of of our drivers clamp the mtu to 1500 - see CR 6618011. As for the 9000 limit 9,000 bytes is the conventional jumbo frame size. There are issues to going to Super Jumbo Frame: see http://en.wikipedia.org/wiki/Jumbo_Frames > * 2047: Comment explaining the structure of buf and how this size is > determined. For example, I can't guess what MAXLINELEN is doing in > there. I've added comments, and in the process of doing so, realized that MAXLINELEN was actually unnecessary. > * 2053: This looks very fragile. It might be easier to follow if you > had a valptr pointer which you initially set to > > (buf + sizeof (char *) * DLADM_MAX_PROP_VALCNT) > > before the loop, and incremented independently by DLADM_PROP_VAL_MAX > in the loop. You could then just set prop_vals[i] to valptr. Actually this code is replicated in multiple parts of libdladm, and I'd like to leave it as is, so that it can all be fixed in one shot later, maybe even captured into a common function. > > * 2058: This would be simpler as: > > status = pdp->pd_get(pdp, linkid, prop_vals, &cnt, media, DLD_DEFAULT); > if (status == DLADM_STATUS_OK) { > status = i_dladm_set_single_prop(linkid, pdp->pd_class, media, pdp, > prop_vals, cnt, flags); > } > free(buf); > return (status); Ok. > usr/src/uts/common/io/bge/bge_main2.c > ------------------------------------- > > * 1062-1256: This function (and its friend bge_get_priv_prop()) would > benefit from a table of per-property default values to reduce the > explosion of nearly duplicate code. I had a discussion with the NIC-China team about this. Having a table is not a general solution for all drivers, because, for example, in the case of e1000g, the hardware bits in the driver have to be read to figure out the default. Even in the bge case (where all supported hardware types have the same defaults in many cases) there are dependancies on whether the hardware is Copper/Fiber (i.e., CHIP_FLAG_SERDES checks). We'll think about this further, but so far, there doesn't seem any clear-cut way to shrink this logic.. > > * 3450: The reference to mac_add_name() is out of place here. Driver > developers shouldn't need to to care about a private implementation > detail of the mac module. fixed (as a result of other changes to this interface). See comments about mac_register_priv_prop > usr/src/uts/common/io/bge/bge_ndd.c > ----------------------------------- > > * 93: I don't understand the reference to the parameter being writable > when this is a get function. Can you explain? relic from the previous ndd code which used to use a single-character encoding at the start of the ndd param name to track writability of the property. Comment has been fixed. > usr/src/uts/common/io/mac/mac.c > ------------------------------- > > * 1059,1064: I don't think that this does any good. Only Solaris > developers use DEBUG kernels, and this message is targetted at > administrators who either use ndd or have written scripts that use > ndd. The message is also not useful in itself. There's no way to > correlate the message with a specific command that was used, so > someone will see this in syslog and wonder where it came from. I > would suggest hacking something into the ndd command itself so that > the thing issuing the command gets something printed to its stdio, > and not a syslog message on a debug kernel. This is a hot topic for debate. Fixing it in ndd is not a good solution because we don't want to print the message for drivers that have not been converted yet, and besides, there are applications like sunvts that issue the ndd ioctls directly, which would also benefit from the message. Dropping the message completely is also not totally satisfactory, as Garrett pointed out, because it's probably good to alert people about the change if possible. Leaving the message in non-debug kernels will result in syslog flooding, so that's not a good solution either. I chose the compromise of putting the message in debug kernels only, but perhaps completely dropping the message is a better approach. mac.c: > * 1883,1888: The idea that these are registered by > mac_register_priv_prop(), and unregistered using mac_unregister() is > asymetric. It would be cleaner if drivers could provide a list of > private properties in mac_register_t using mac_register(), much like > they provide a list of callbacks. mac_ndd.c: > * 516: I don't see why this can't be replaced by a table that's passed > in mac_register_t. Is there a reason why we chose on registering > individual properties like this? Actually, you may have missed the subsequent email discussion where the mac_register_priv_prop interface was changed. See http://mail.opensolaris.org/pipermail/brussels-dev/2008-March/000795.html However, as you point out, even this interface can be optimized further by just passing the table as part of mac_register. So the comment is accepted. > > usr/src/uts/common/io/mac/mac_ndd.c > ----------------------------------- > > * 67-161: These are all Ethernet specific. The mac module is designed > to be Media agnostic, which is why we introduced a MAC-Type plugin > architecture. It looks like there's some missing architecture to > make use of the plugin interfaces and the mac_ether plugin, and it > doesn't look like it would be difficult to implement that. Plugins > already register a table of media-specific statistics, and this is > not at all that different. It's also shouldn't be all that > different from supporting a table of private properties registered > by the driver. > > * 163: This isn't the size, but a count. (MAC_MII_PROP_COUNT) I can easily make this to be part of the mactype plugin, but there is one danger to this though: the ndd interface is currently only used to diddle the MII props of ethernet interfaces. When we put in hooks for the ndd mapping table into the mactype_register_t we provide a way for other linktypes to use ndd to map from ndd invocations to dladm invocations. I don't see anyone reasonably wanting to use this backdoor, but it does provide another new way to use ndd. > * 192,198,521: treating pointers as booleans. Ok. > * 194: why 5? > * 243: do we know that this isn't past the end of the dblk? As I was parsing the above two comments, I realised there's a possible bug in line 243: that should be 6, and it comes from the count of space, (, ) and terminating nulls in (void) snprintf(cp, len, "%s (%s)", name, rwtag); I've clarified this with comments. > * 270,276: there is no need for the "goto done" nor for the "done" > label as it currently is, as the code falls through to 277 without > the goto. IMO, it would be simpler to have a single "done" label > (and not err_ret), which looks like: > > done: > if (err == 0) > miocack(wq, mp, msgdsize(mp->b_cont), rval); > else > miocnack(wq, mp, 0, err); > > You could then turn 261 into "goto done", and remove all of the > other goto's in this function. Ok. > * 340: this ASSERT() looks bogus, but maybe I'm not looking closely > enough. What's to prevent random joe admin from panicking the > system by typing ndd -get ... on a driver which doesn't supply a > getprop() callback? Actually we check for this in mac_ioctl (which checks the mc_callbacks), so the ASSERT should hold. > * 361: I don't like the default here. This should be an explicit 8, > and the default should be to ignore the property. If there is > sanity checking on properties when they're registered, then an > ASSERT() would also be sensible. Ok. > * 381: This is fragile, and will break when someone adds another > 64-bit property and doesn't realize that this code exists. The important point to keep in mind is that the ndd<->driver interface itself can only set/get 32 bit values which is another limitation to using ndd for drivers. Which is why the speed is scaled down for kstat/ndd reporting. Since we don't expect to add any more parameters to ndd itself the above danger does not hold for this path. > * 401-425: These different return paths via a break in a loop and > three different goto labels is way too complex for my taste. I > can't determine what its doing, so I cannot effectively review it to > determine if its doing the right thing. Can this please be > simplified? Ok. > usr/src/uts/common/sys/mac.h > ---------------------------- > > * 561: suggest a property-specific prefix like MAC_PROP > Ok (this last one is not fixed in the new webrev yet, but will do so)
