Date: Tue, 19 Aug 2008 16:02:21 -0600 (MDT) From: Warner Losh <imp@bsdimp.com> To: hselasky@c2i.net Cc: freebsd-usb@freebsd.org Subject: Re: usb4bsd patch review Message-ID: <20080819.160221.41717240.imp@bsdimp.com> In-Reply-To: <200808192228.39015.hselasky@c2i.net> References: <48AB233C.2010602@FreeBSD.org> <20080819200017.GC16977@elvis.mu.org> <200808192228.39015.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
From: Hans Petter Selasky <hselasky@c2i.net>
Subject: Re: usb4bsd patch review [was Re: ...]
Date: Tue, 19 Aug 2008 22:28:36 +0200
> > >
> > > What do the newbus guys say about this? Adding a workaround in
> > > underlying code for a problem caused by your own code is often a signal
> > > that you're not going about it the right way. At the very least the
> > > reason for the special case should be documented here.
> >
> > I need to think about this, Hans gave me a better argument on
> > AIM earlier for it, I need to reload this in my head.
> >
> > > >int
> > > >device_delete_all_children(device_t dev)
> > > >{
> > > > device_t *devlist;
> > > > int devcount;
> > > > int error;
> > > >
> > > > error = device_get_children(dev, &devlist, &devcount);
> > > > if (error == 0) {
> > > > while (devcount-- > 0) {
> > > > error = device_delete_child(dev,
> > > > devlist[devcount]);
> > > > if (error) {
> > > > break;
> > > > }
> > > > }
> > > > free(devlist, M_TEMP);
> > > > }
> > > > return (error);
> > > >}
> > > >
>
> In the existing kernel code, "device_get_children()" is used many places
> without checking the error code. I have patches, but they are not part of the
> patch-set.
>
> Also freeing a pointer to zero bytes is not logical. I'm not sure if this is
> allowed in kernel space?
>
> ptr = malloc(0, ... )
> free(0);
>
> The device_get_children() could have returned an error if there are no
> children, but again, the existing code does not check this return value.
The existing code is fine when there's no children. The semantics of
the kernel allocation routines are such that there's no problem. I
don't think there's a bug here at all. The bug here is that the
interface is impossible to lock, but that's another issue....
Having said that, I rather like the idea of having a
device_delete_all_children.
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080819.160221.41717240.imp>