Date: Thu, 27 Jul 2006 16:30:09 +0100 (BST) From: Robert Watson <rwatson@FreeBSD.org> To: Sam Leffler <sam@errno.com> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/usb if_ural.c Message-ID: <20060727162621.X17345@fledge.watson.org> In-Reply-To: <44C8DA23.5000406@errno.com> References: <200607260330.k6Q3UooP047077@repoman.freebsd.org> <20060727124542.V4612@fledge.watson.org> <44C8DA23.5000406@errno.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 27 Jul 2006, Sam Leffler wrote:
>> ---------------------------------------
>> @@ -209,6 +211,9 @@
>> */
>> if (ic->ic_reset == NULL)
>> ic->ic_reset = ieee80211_default_reset;
>> +
>> + KASSERT(ifp->if_spare2 == NULL, ("oops, hosed"));
>> + ifp->if_spare2 = ic; /* XXX temp backpointer */
>> }
>> ---------------------------------------
>>
>> Please don't use spare pointers without properly allocating them (i.e.,
>> renaming them). I ran into this this morning when merging these changes
>> into the rwatson_ifnet branch, where if_spare2 was renamed to if_startmbuf.
>> However, if the above change is MFC'd as is, people may well find the
>> problem through run-time corruption when the network stack tries to execute
>> the ic contents thinking that it's a non-NULL if_startmbuf function
>> pointer, which is a lot harder to debug. The point of spare fields is that
>> they be spare -- if they are no longer spare, they should not be left
>> marked as spare, or they will get reused with unfortunate ABI consequences.
>> I don't mind if this spare is allocated (if_softc2, if_driver, or the
>> like), as we have one more spare pointer I can use in 6.x, but the current
>> use of the spare field is problematic and should not be MFC'd unless
>> cleaned up.
>
> This was intended as a stopgap until the vap changes were in place and will
> never be mfc'd. I understand about reuse; hence the assertion. Since you
> renamed the field didn't code just stop compiling? I assumed these were
> going to be treated like the M_PROTOx flags in mbuf.h that are kinda spare
> but used within layers for their own purpose.
That wasn't the intent, but it's not clear that the intent was ever documented
(other than in the commit message). The original intent was to provide
pointers that could be reused as part of ifnet reworking without breaking the
ABI in 6.x. I'm happy if we want to designate one of these pointers for use
by the driver, but if we want to do that, we should just rename it to
if_driver or if_softc2 or the like to make sure its use is clear. How about
we rename if_spare* to if_unused*, and rename if_spare2 to if_driver?
The ABI worry is what happens with loadable modules -- that you load a 6.x
module from 6.2 on 6.3 and discover that things are unhappy because suddenly
the ifnet framework is invoking the function pointer that is otherwise NULL
(which may be a slight stretch).
Robert N M Watson
Computer Laboratory
University of Cambridge
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060727162621.X17345>