Re: git: 7b3ee39e73af - main - libcam: Include nvme opcode and status code routines from nvme_util.c

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