proposal: require ivar accessors to succeed

[Warner Losh]

On Mon, May 27, 2019, 2:47 PM Andriy Gapon <avg at freebsd.org> wrote:

> On 27/05/2019 21:10, Bjoern A. Zeeb wrote:
> > On 27 May 2019, at 5:44, Andriy Gapon wrote:
> >
> >> __BUS_ACCESSOR() macro is used to define accessors to bus IVAR
> variables.
> >> Unfortunately, accessors defined in such a fashion completely ignore
> return
> >> values of BUS_READ_IVAR() and BUS_WRITE_IVAR() method calls.  There is
> no way to
> >> see if a call is successful.  Typically, this should not be a problem
> as a
> >> device driver targets a specific bus (sometimes, buses) and it should
> know what
> >> IVARs the bus has.  So, the driver should make only those IVAR calls
> that are
> >> supposed to always succeed on the bus.
> >> But sometimes things can go wrong as with everything else.
> >>
> >> So, I am proposing to add some code to __BUS_ACCESSOR to verify the
> success.
> >> For example, we can panic when a call fails.  The checks could be under
> >> INVARIANTS or under DIAGNOSTICS or under a new kernel option.
> >> A less drastic option is to print a warning message on an error.
> >>
> >> This is mostly intended to help driver writers and maintainers.
> >>
> >> Opinions, suggestions, etc are welcome.
> >
> > What about “fixing” the KPI (possibly adding a 2nd one), deprecating the
> old
> > one, and (slowly over time) migrating old stuff over?
>
> I think that the two proposals are not mutually exclusive.
> And I think that both make sense.
> However, it's hard for me to imagine a desire to replace code like this
>   devid = pci_get_devid(dev);
> with this
>   err = pci_get2_devid(dev, &devid);
>   if (err != 0) {
>     ...
>   }
>
> Especially given that, modulo bugs, dev is going to be a device on the pci
> bus
> and the call is going to succeed.
> In other words, in my opinion, the only cases where an accessor is allowed
> to
> fail are:
> - a driver somehow attached to a device on an unexpected bus
> - uncoordinated changes in between a bus driver and a device driver
> So, programming errors.
>

I'm cool with panic. The accessor functions are all supposed to be can't
fail. And creating a new set of APIs that can return failure for can't fail
things will just bloat the code with cargo cult error handler's that add no
value.

So put me down on the NO to the new API. If you want to test if the ivar is
supported, use the lower level READ_IVAR. Let's not break the API because
one person had a bug...

Warner


> --
> Andriy Gapon
> _______________________________________________
> freebsd-arch at freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe at freebsd.org"
>