Page MenuHomeFreeBSD

Use C errno as return value for cam_periph_acquire()
ClosedPublic

Authored by scottl on Jan 26 2018, 10:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Oct 2 2024, 2:05 PM
Unknown Object (File)
Sep 16 2024, 7:57 PM
Unknown Object (File)
Sep 11 2024, 4:01 PM
Unknown Object (File)
Sep 5 2024, 9:14 PM
Unknown Object (File)
Aug 13 2024, 5:22 PM
Unknown Object (File)
Jun 27 2024, 4:26 AM
Unknown Object (File)
Jun 25 2024, 1:10 AM
Unknown Object (File)
Jun 6 2024, 4:37 PM
Subscribers
None

Details

Summary

There's no compelling reason to return a cam_status type for
cam_periph_acquire(), and doing so only creates confusion with normal C
coding practices. It's technically an API change, but the periph API
isn't widely used. No efffective change to operation.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Jan 26 2018, 10:11 PM
ken requested changes to this revision.Feb 1 2018, 11:39 PM

The change in general is fine. It just needs fixing in the ctlferegister() case.

sys/cam/ctl/scsi_ctl.c
546 ↗(On Diff #38498)

This isn't quite right. ctlferegister() is defined to return cam_status. The status variable is set above to CAM_REQ_CMP, and then reset to various things as we go through the loop.

Here, you're returning the status value that probably reflects success, but it should reflect a failure to acquire.

So, how about returning CAM_REQ_CMP_ERR here.

This revision now requires changes to proceed.Feb 1 2018, 11:39 PM
sys/cam/ctl/scsi_ctl.c
546 ↗(On Diff #38498)

Good catch, thanks.

I neither have major objections aside of one inline, nor too happy. From one side I agree that there is no any/much sense to return cam_status from functions like that, which do not work with command statuses. From the other side it is a) an API change, which may lead to difficult to find errors, but b) there are many other functions suffering the same problem (but which may be more invasive to fix though).

sys/cam/cam_periph.c
409 ↗(On Diff #38498)

Why still cam_status here? Should be an int probably.

sys/cam/cam_periph.c
409 ↗(On Diff #38498)

Good catch, I'll fix it.

In D14063#298088, @mav wrote:

I neither have major objections aside of one inline, nor too happy. From one side I agree that there is no any/much sense to return cam_status from functions like that, which do not work with command statuses. From the other side it is a) an API change, which may lead to difficult to find errors, but b) there are many other functions suffering the same problem (but which may be more invasive to fix though).

This is just the first part of a larger project to clean up CAM. Some of the other functions that you refer to will be fixed after this.

This revision was not accepted when it landed; it landed in state Needs Revision.Jun 26 2018, 9:55 PM
This revision was automatically updated to reflect the committed changes.