Sowmini,
Sowmini.Varadhan at Sun.COM wrote:
>> Extnernal webrev:
>> http://cr.opensolaris.org/~sowmini/nddcompat_review/
>> Internal webrev:
>> http://zhadum.east/export/ws/sowmini/brussels/nddcompat_review
>>
>> cscopes:
>> /net/zhadum.east/export/ws/sowmini/brussels/nddcompat-review/usr/src[/uts]
>>
Here are my comments:
usr/src/lib/libdladm/common/linkprop.c
--------------------------------------
* 210: This constant should be DLD_PROP_MTU to match the property
name.
* 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.
* 1017: No need for the brackets.
* 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).
* 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?
* 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.
* 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.
* 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);
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.
* 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.
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?
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.
* 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.
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)
* 192,198,521: treating pointers as booleans.
* 194: why 5?
* 204: you could just do "+ 1" at 203 instead.
* 243: do we know that this isn't past the end of the dblk?
* 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.
* 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?
* 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.
* 381: This is fragile, and will break when someone adds another
64-bit property and doesn't realize that this code exists.
* 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?
* 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?
usr/src/uts/common/sys/mac.h
----------------------------
* 561: suggest a property-specific prefix like MAC_PROP
-Seb
