On 6/5/25 22:21, Warner Losh wrote:
On Thu, Jun 5, 2025 at 7:41 PM John Baldwin <j...@freebsd.org> wrote:
On 6/5/25 21:32, John Baldwin wrote:
The branch main has been updated by jhb:
URL:
https://cgit.FreeBSD.org/src/commit/?id=7b3ee39e73af36f49f471f7900baeb98ac3504d0
commit 7b3ee39e73af36f49f471f7900baeb98ac3504d0
Author: John Baldwin <j...@freebsd.org>
AuthorDate: 2025-06-06 01:28:38 +0000
Commit: John Baldwin <j...@freebsd.org>
CommitDate: 2025-06-06 01:28:38 +0000
libcam: Include nvme opcode and status code routines from nvme_util.c
libcam in userspace also includes nvme_all.c which now depends on
nvme_util.c, so add nvme_util.c to libcam's sources. This requires
exporting the opcode and status code routines in nvme_util.c to
userspace as well as the kernel. In turn, this means nvmecontrol now
depends on libsbuf (which is already present in /lib).
Reported by: viswhin, Jenkins
Fixes: 60159a98a837 ("nvme: Move opcode and status code tables from
base CAM to nvme_util.c")
Sponsored by: Chelsio Communications
This fixes the build for now (and sorry for breaking it). However, this
raises a few questions for me at least. Why does libcam include nvme_all.c
at all? We don't document any of the nvme_* functions in cam(3), nor any
of the functions from scsi_all.c, smp_all.c, etc. This seems really odd,
and it also means that we can add (and remove!) symbols from libcam without
realizing it by changing sys/cam/<proto>/<proto>_all.h which is not very
obvious.
It seems to me that we should be more intentional about which symbols we
export from libcam. Switching to symbol versioning (which implicitly
enforces hidden visibility on all symbols not explicitly exported) would
keep us from leaking symbols into the ABI of libcam that we don't intend
to export.
I also wonder if we can remove some of the *_all.c files from libcam
entirely? None of the nvme_* ones are used outside of the kernel in the
base system for example.
No. We can't remove all that. They are used by camcontrol, ddcam and
others. We use the functions in *_all.h/c functions to populate the
CCBs to send down into the kernel. I think that we need a different
take on it. It is, after all, basically designed in FreeBSD 3
timeframe when such considerations were the furthest thing from the
minds of the developers. Times have changed, though, and libcam hasn't
with it.
We don't use a single routine from nvme_all.c outside of the kernel.
libcam also doesn't include mmc_all.c.
We don't document it very well... Sometimes I've thought we should
make libcam a private library. But I don't know who all uses it in raw
mode. I think about a dozen ports use this for things like smart
reporting...
I think a decent start would be to maybe be a bit more choosy in what
things have to be exported. Only a few routines in ata_all.c and
smp_all.c appear to be used in camcontrol for example. And it
certainly seems like we could remove nvme_all.c from libcam entirely.
Using a version map with symbol versioning might be the best way to
enforce that (though more liberal use of #ifdef _KERNEL in *_all.c
would be another route). We definitely will need to bump the
SHLIB_MAJOR for libcam before branching 15 though.
--
John Baldwin
