Re: git: 7b3ee39e73af - main - libcam: Include nvme opcode and status code routines from nvme_util.c
- Reply: Warner Losh : "Re: git: 7b3ee39e73af - main - libcam: Include nvme opcode and status code routines from nvme_util.c"
- In reply to: Warner Losh : "Re: git: 7b3ee39e73af - main - libcam: Include nvme opcode and status code routines from nvme_util.c"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Fri, 06 Jun 2025 14:34:11 UTC
On 6/5/25 22:21, Warner Losh wrote: > On Thu, Jun 5, 2025 at 7:41 PM John Baldwin <jhb@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 <jhb@FreeBSD.org> >>> AuthorDate: 2025-06-06 01:28:38 +0000 >>> Commit: John Baldwin <jhb@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