On Fri, Jun 6, 2025 at 8:34 AM John Baldwin <j...@freebsd.org> wrote: > > 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.
Hmmm, I added it into libcam because we did use one. But maybe we don't need it. It's been a while, and I added it in my experimental tree and push that upstream before I pushed the things that used it, so maybe something got dropped. > > 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. I'll give that a short. I was sure that something was using it, but that may have only been in early version(s) of code... I'll look into it. > 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. Yea. I've been hesitant to clean this up too much. I know $WORK doesn't need this library for anything interesting. But I also think we should check with Netapp and Spectralogic. Warner
