Skip to content

MI: return value + errno unification#418

Merged
igaw merged 3 commits intolinux-nvme:masterfrom
CodeConstruct:mi-retvals
Jul 7, 2022
Merged

MI: return value + errno unification#418
igaw merged 3 commits intolinux-nvme:masterfrom
CodeConstruct:mi-retvals

Conversation

@jk-ozlabs
Copy link
Copy Markdown
Collaborator

This series implements a unification of the return values across the MI code, so that we have consistent semantics between libnvme and libnvme-mi.

From the final patch:

This gives us the following semantics:

  • zero on success

  • -1 for errors in the MI communication with an endpoint, with errno
    set accordingly

  • positive values where the MI communication succeeded, but we received
    an error response from the endpoint. The return value will be that
    from the MI response status field, and should correspond to one of
    the nvme_mi_resp_status values.

Patch 1 extends the defined MI status values, so we're complete with what's in the current spec, and patch 2 implements a small fix to the error handling when we have duplicate endpoints during a scan.

As always, comments and feedback most welcome. Let me know if this is a suitable equivalent to the libnvme core API semantics.

jk-ozlabs added 3 commits July 7, 2022 13:55
We'll want to make further use of the MI status values in an upcoming
change, so add the full set of values from NVMe-MI v1.2b.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
It's not an error to have an endpoint in our list prior to scanning;
just suppress the duplicate and keep going.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
This change unifies and documents the error return values for the MI
implementation. This gives us the following semantics:

 - zero on success

 - -1 for errors in the MI communication with an endpoint, with errno
   set accordingly

 - positive values where the MI communication succeeded, but we received
   an error response from the endpoint. The return value will be that
   from the MI response status field, and should correspond to one of
   the nvme_mi_resp_status values.

We add these semantics to the file-level kdoc comments in mi.h.

Most of the changes here are replacing the negative-errno returns:

    return -EIO;

with:

    errno = EIO;
    return -1;

but there are a few slightly-more-involved changes where we need to
preserve errno across a cleanup/log that might clobber it.

For the dbus code in mi-mctp.c, we need to convert the sd_bus convention of
negative-errno values into errno; we can do most of these through the
dbus_err() helper.

Fixes: linux-nvme#417

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
@jk-ozlabs jk-ozlabs marked this pull request as ready for review July 7, 2022 08:30
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #418 (4473661) into master (ea95233) will increase coverage by 0.24%.
The diff coverage is 35.00%.

@@            Coverage Diff             @@
##           master     #418      +/-   ##
==========================================
+ Coverage   16.16%   16.41%   +0.24%     
==========================================
  Files          31       31              
  Lines        5010     5074      +64     
  Branches      973      974       +1     
==========================================
+ Hits          810      833      +23     
- Misses       3948     3988      +40     
- Partials      252      253       +1     
Impacted Files Coverage Δ
src/nvme/mi.h 36.84% <ø> (ø)
src/nvme/mi.c 51.94% <34.93%> (-2.21%) ⬇️
src/nvme/mi-mctp.c 70.40% <35.29%> (-4.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea95233...4473661. Read the comment docs.

@igaw igaw merged commit 4e38073 into linux-nvme:master Jul 7, 2022
@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Jul 7, 2022

Thanks!

@jk-ozlabs jk-ozlabs deleted the mi-retvals branch July 28, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants