On Fri, 2008-08-15 at 11:48 -0400, Peter Memishian wrote:
> > 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.
Agreed, I was more grumbling to myself than suggesting doing something
about it in this wad. :-)
Back to dld_drv.c:
> > > * 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.
Yes, "driver instance" is implied since there's only one instance, and
thus all state is driver-global. There's also this bit of weirdness in
the original _fini():
162 int
163 _fini(void)
164 {
165 int err;
166
167 if ((err = mod_remove(&drv_modlinkage)) != 0)
168 return (err);
169
170 if (drv_fini() != 0) {
171 (void) mod_install(&drv_modlinkage);
172 return (DDI_FAILURE);
173 }
174
175 return (err);
176 }
This is wrong. One should only return EBUSY prior to calling
mod_remove(), and only teardown global resources after calling
mod_remove(). In other words, you do things that can fail before
mod_remove(), and things that can't fail after_mod_remove() (because
mod_install() can actually fail above, then where are you?). As such
the fix is either to dissect drv_fini() into pieces that can be called
before and after mod_remove(), or simply attempt the teardown in the
global instance detach(). The latter is simpler IMO.
> > 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.
That's what I ended up doing.
> > > 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.
Indeed. I also looked for something like this, and didn't find it.
In dld_ioc.h:
> > >
> > > * 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 :-)
Okay, I've clarified the comment.
Thanks,
-Seb
_______________________________________________
networking-discuss mailing list
[email protected]
