Page MenuHomeFreeBSD

cam: fix xpt_bus_register and xpt_bus_deregister return errno
ClosedPublic

Authored by imp on Jun 21 2021, 9:03 PM.

Details

Summary

xpt_bus_register and xpt_bus_deregister returns a hybrid error that's
neither a cam_status, nor an errno, but a mix of both. Update
xpt_bus_register and xpt_bus_deregister to return an errno. The vast
majority of current users compare against zero, which can also be
spelled CAM_SUCCESS. Nobody uses CAM_FAILURE, so remove that symbol
to prevent comfusion (nothing returns it either).

Where the return value is saved, ensure that the variable 'error' is
used to store an errno and 'status' is used to store a cam_status where
it makes the code clearer (usually just in functions that already mix
and match). Where the return value isn't used at all, avoid storing it
at all.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp requested review of this revision.Jun 21 2021, 9:03 PM
mav requested changes to this revision.Jun 23 2021, 6:29 PM

First, I see this change as extremely invasive with pretty small benefit.
Second, I also not sure what benefit of using cam_status here, where it is not about doing I/O, that most of statuses represent.
Third, in 99e7a4ad9e6fe53868cb15449667ad46814d692b Scott switched cam_periph_acquire() from cam_status to errno, so this change goes wrong direction.

I'd prefer switch to errno instead here, that IMO is more logical and would keep existing KPI/KBI due to CAM_SUCCESS == 0.

This revision now requires changes to proceed.Jun 23 2021, 6:29 PM

I'd picked cam_status as that is what a couple of existing SIMs expected that, and that is what had been returned before when CAM_SUCCESS wasn't the return value. I do like errno even better. I'll get rid of CAM_FAILURE, though, since it was never used and was never right to use here.

Thanks

start to apply mav's feedback

imp retitled this revision from cam: fix xpt_bus_register and xpt_bus_deregister return cam_status to cam: fix xpt_bus_register and xpt_bus_deregister return errno.Jun 23 2021, 8:28 PM
imp edited the summary of this revision. (Show Details)

This should address mav's concerns.

don't need to really update ps3 cdrom driver as part of this.

I like the scope of this change better. Thanks for the suggestion mav@. IT's also MFC-able.

mav added inline comments.
sys/cam/cam_xpt.c
936

Wouldn't it be better to return the error here?

sys/cam/cam_xpt_sim.h
44

The same should probably be done for xpt_bus_deregister() for symmetry. It is not so broken and it will be a change of KPI, but its return code seems really checked only by pvscsi and umass.

This revision is now accepted and ready to land.Jun 23 2021, 8:37 PM

actually update xpt_bud_deregister too... Nobody is checking the return value so
it doesn't matter from a compat point of view, but why return anything?

This revision now requires review to proceed.Jun 23 2021, 9:59 PM
In D30860#694619, @imp wrote:

Nobody is checking the return value

pvscsi and umass check it for CAM_REQ_CMP and should be updated.

mav caught two places I missed where xpt_bus_deregister checks the return value.

Yeah, my feeling is still that C function return values should be errno's, and that cam_status should be reserved for use primarily in the status fields of CCBs and related structures. If a C function needs to return a cam_status, it should return an errno and pass the cam_status by reference.

This revision is now accepted and ready to land.Jun 24 2021, 1:47 AM