Meem, Thanks for the review, my responses are in-line below.
I've made all necessary updates to the code and generated two new webrevs to reflect this: 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/ On Wed, 2008-08-13 at 04:53 -0400, Peter Memishian wrote: > cmd/dlmgmtd/dlmgmt_main.c: > > * 50: Do we still need to #include <stropts.h>? No; removed. > > lib/libdladm/common/libdladm.c: > > * 29: Likewise. Removed. > > lib/libdladm/common/libdlaggr.c: > > * 54: Why keep DLADM_AGGR_DEV? It seems to just obscure > what's actually going on. Indeed, it's only used in one place anyway. I've removed it. > > * 241: What's the point of this reinitialization, when it > will occur again at line 228? There is no point; removed. > > * 656-657: `rc' seems needless. It is; removed. I've also removed it from i_dladm_aggr_info_active() for the same reason. > > lib/libdladm/common/libdlvnic.c: > > * 49: As above, why keep VNIC_DEV? Initially it was to reduce the amount of noise in the webrev, but on second thought, I've removed it. > * 85-87, 186-188: `rc' seems needless Removed. > > * 121-122: Not your bug, but this code is wrong, since > `errno' may be damaged by the call to close(). Fixed, and removed "rc" in the process. > > lib/libdladm/common/linkprop.c: > > * 2030: Not your bug, but initializing `status' to the > return value here seems wrong (thankfully, DLADM_STATUS_OK > is zero). Fixed. > > lib/libdladm/common/secobj.c: > > * 58: I suspect SECOBJ_BUFSZ was an artifact of the old I_STR > implementation (a la `gbuf' in ndd.c). Now that normal ioctls > are being used, it'd seem preferable to remove this arbitrary > limit and instead keep increasing the buffer size by multiples > of 2 until it fits. Accepted, I'll implement this. > > * 200: I don't understand this change -- why does the value > of sg_size matter in a normal (non-walk) `get' operation? It shouldn't, and this should actually be sizeof (secobj_get). Fixed. > > pkgdefs/common_files/i.devpolicy: > > * 55: Unclear what the change to `aggr' here has to do with this > wad. Nothing; this was a spurious change. I've reverted it. > 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... > uts/common/io/aggr/aggr_ctl.c: > > * 118, 241: A nit, but `out' or `done' seems a more appropriate > label given that we go through this on the normal return path. Fixed (changed to "done"). > > * 213: Seems like `karg' should just be an `laioc_add_rem_t *'. Fixed. > > * 242: How do we get here with ports == NULL? We can't; Fixed. > > usr/common/sys/aggr.h: > > * Although the changes regarding to 32/64-bit structure layouts > look safe, I think a comment should be added to ensure that > developers keep in mind that aggr driver assumes that the > structures will have identical layout when compiled either ILP32 > or LP64. Done. > > uts/common/io/dld/dld_drv.c: > > * 71: Unrelated to your changes, but it seems like `dld_dip' > should be `static'. Fixed. > > * 93: D_NEW does nothing. Removed. > > * 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. 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!). > * 310-318: Seems like you could just define the close entrypoint > as `nulldev'. Yum, more lines to remove. Done. > * 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)? > * 533: Not your change, but why do we return `EINVAL' rather > than `err' here? Fixed. > > * 603-605: Code repeats check on line 606. Fixed. > > * 925: Doesn't this need to subtract off > `sizeof (dld_ioc_secobj_get_t)'? Yes, it does. Since this is overly huge, we've never run into the end of this buffer. Fixed. > * 935: Why not remove line 933 and just `return (state.ss_rc)'? > Fixed. > * 1014-1015: Comment doesn't seem to completely match code (cf. > line 1019). Right; dld doesn't need to call dld_ioc_register() since it can statically initialize its ioctl list. I've fixed the comments and added a block comment above dld_ioc_modtable to clarify this. > > * 1044: Maybe ENOENT? Okay; fixed. > > * 1054: Perhaps we should VERIFY() the call returns 0? Good idea; fixed. > > * 1081: Seems like `modid' could just be inlined into line 1089 > without losing readability. Okay; fixed. > > * 1114, 1122: No need to test `!= 0' in di_flags checks. Fixed. > > * 1094: Need to separate cases and `ddi_release_devi(dip)' in the > second case. Fixed. > > * 1102, 1108: Need to `ddi_release_devi(dip)'. Fixed. > > 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. I'll double check with Fei Feng to make sure that the implication of this change is understood. > > uts/common/io/ipw/ipw/ipw2200.c: > > * 2408: Likewise. See above. > > uts/common/io/vnic/vnic_ctl.c: > > * 52: Why is VNIC_IOC_CREATE DLDCOPYINOUT rather than just > DLDCOPYIN? That was a typo. Fixed. > > * 62-63: Given that this is no longer a STREAMS driver, it seems > inappropriate to use DDI_DEFINE_STREAM_OPS. Fixed. > > * 183: s/VNICIOC/VNIC_IOC/ Fixed. > > * 300: I'm confused by this -- seems like we only copied in a > vnic_ioc_info_t into info_argp, but we set `where' to > &info_argp[1]. Also, where's the copyout() in > vnic_ioc_info_new_vnic()? This ioctl indeed is broken. I've fixed it. I hadn't yet tested with Xvm, and I obviously need to. > > uts/common/os/policy.c: > > * 1709-1710: cstyle violation. Fixed. > > uts/common/os/priv_defs: > > * 390: s/SYS_DATALINK/SYS_DL/ Fixed. > > uts/common/sys/dld.h: > > * 221: While you're here, remove this extra blank line. Fixed. > > uts/common/sys/dld_ioc.h: > > * 26: Seems like these guards should be _SYS_DLD_IOC_H Fixed. > > * 30: Extra blank line. Fixed. > > * 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? > * 81: I'd recommend typedefing the function (rather than the > pointer to the function), then making the pointer explicit in > the di_func definition. Then the typedef can be used for > declaring functions matching the callback signature if need be. Fixed. > > * 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. > 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. > (Also, a nit, but given the name of the > privilege, DLDDLCONFIG seems like a preferable name.) Fixed. > > * Throughout: s/ammount/amount/ That one is my fingers' fault. ;-) Fixed. > > uts/common/sys/vnic.h: > > * 35: What needed <sys/dls.h>? Evidently nothing. Removed. > * Although the changes regarding to 32/64-bit structure layouts > look safe, I think a comment should be added to ensure that > developers keep in mind that vnic driver assumes that the > structures will have identical layout when compiled either > ILP32 or LP64. Done. > > uts/intel/os/device_policy: > > * 63-64: Unclear what aggr/vnic changes have to do with this > wad. I've reverted this. It's not related. > > uts/sparc/os/device_policy: > > * 66-67: Likewise. Same as above. Thanks! -Seb _______________________________________________ networking-discuss mailing list [email protected]
