> A full webrev against onnv: > http://cr.opensolaris.org/~seb/dladm-privs.2/ > > An incremental webrev against the first webrev: > http://cr.opensolaris.org/~seb/dladm-privs.2.inc/ > > > uts/common/io/aggr/aggr_dev.c: > > > > * 46-47: Given that this is no longer a STREAMS driver, it seems > > inappropriate to use DDI_DEFINE_STREAM_OPS. > > Fixed, but that sure is a handy macro. Too bad it has the word > "STREAMS" in it for no apparent reason...
Well, I certainly wouldn't be opposed to a general (uncommitted) DDI_DEFINE_CHARDRV_OPS macro that does what you'd want. There may be some additional fields in the dev_ops or cb_ops that some character driver writers would like to specify, so a little bit of research seems warranted if we go in that direction. > > * 128, 134: Not that I object, but what was the rationale for > > moving drv_init()/drv_fini() from _init()/_fini() to attach()/ > > detach()? > > It's because the drv_fini() is used to check to see if the driver is in > use and can return EBUSY. Replace "driver" with "driver instance" and I agree with the above. I don't see an inherent problem with having _fini() fail with EBUSY if there are driver-global resources that are in-use. > That kind of semantic belongs in detach(). For symmetry, drv_init() > was moved to the attach() routine. This also fixes an existing bug > which is that dld would not teardown with drv_fini() in _init() if > mod_install() failed (how could it, drv_fini() could fail itself!). > > > * 418: I'm not entirely sure how kmem_alloc() copes with an > > allocation of zero bytes, but if pr_valsize is sufficiently > > large, the calculation on line 418 may cause dsize to be zero. > > Might want to check for this case. > > I can do something like the following to catch any wraparound: > > dsize = sizeof (dld_ioc_macprop_t) + dipp->pr_valsize - 1; > if (dsize < dipp->pr_valsize) > return (SOMETHING); > > But I'm not sure what SOMETHING would be. EINVAL (because pr_valsize > cannot possibly be that big in reality)? I'd think so. > > uts/common/io/ipw/ipw/ipw2100.c: > > > > * 2185: The original code suggests that this source also ran on > > S10; is the WiFi team bought into this change? > > I have asked the WiFi team to participate in this code review, yes. So > far, I've received positive confirmation from Fei Feng. Note that the > code wouldn't have compiled on Solaris versions that didn't have > secpolicy_net_config() anyway, since checking for (secpolicy_net_config > == NULL) wouldn't have compiled. The code was broken to begin with. This usually handled by providing some header-file magic that sets up a #pragma weak for the symbol -- e.g., see the stuff in hxge_impl.h. That said, I don't see the #pragma weak directives in the WiFi driver headers. > > > > * 73: But the code seems to do this at attach() time, not at > > _init() time. > > True, and it doesn't matter when it's done as long as it happens as part > of the ddi_hold_devi_by_instance(<module>, 0, 0) call in drv_ioctl() if > the module isn't loaded. The assumption is that <module> is a pseudo > driver that has one instance, 0. I'm not sure how to make this clearer. > Any suggestions? Your explanantion above seems to suffice :-) > > * 81: It's unclear to me what the final "int *rvalp" argument > > to the dld_ioc_func_t is for. Everyone seems to ignore it. > > Removed. That reminds me, and speaking of currently unused arguments; > if we want these ioctls to work using ldi, then I actually have to use > the mode argument and call ddi_copy*() instead of copy*(). I'll fix > that too. OK. > > Also, would we really expect a caller to directly manipulate > > `cred', rather than putting the cred checks in drv_ioctl() > > ala DLDDLCONF? > > Interesting question. I would think that this allows an individual > ioctl to do something unique with the credp that isn't necessarily > broadly applicable and not necessarily a privilege related thing. I'm > inclined to leave it there. OK. -- meem _______________________________________________ networking-discuss mailing list [email protected]
