Date: Fri, 6 Jun 2025 10:34:11 -0400 From: John Baldwin <jhb@FreeBSD.org> To: Warner Losh <imp@bsdimp.com> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 7b3ee39e73af - main - libcam: Include nvme opcode and status code routines from nvme_util.c Message-ID: <346c0310-b9b6-4eb7-ac94-0575a83c3416@FreeBSD.org> In-Reply-To: <CANCZdfp6-QVoJRD5JsMML0%2BzdMxSo4Od0U3Zq20nVbn1n0BUQw@mail.gmail.com> References: <202506060132.5561Wakc094185@gitrepo.freebsd.org> <eea5514d-2b3b-4491-bc2a-9bbed5bdacbc@FreeBSD.org> <CANCZdfp6-QVoJRD5JsMML0%2BzdMxSo4Od0U3Zq20nVbn1n0BUQw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?346c0310-b9b6-4eb7-ac94-0575a83c3416>