Page MenuHomeFreeBSD

Allow Illumos code to co-exist with nv(9)
ClosedPublic

Authored by rstone on Feb 17 2015, 5:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Feb 4, 8:04 PM
Unknown Object (File)
Jan 3 2023, 11:55 AM
Unknown Object (File)
Jan 2 2023, 5:00 AM
Unknown Object (File)
Apr 14 2017, 2:27 AM
Unknown Object (File)
Apr 12 2017, 9:29 PM
Unknown Object (File)
Apr 11 2017, 6:49 AM
Unknown Object (File)
Apr 4 2017, 3:37 AM
Unknown Object (File)
Feb 20 2017, 1:50 PM

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rstone retitled this revision from to Allow Illumos code to co-exist with nv(9).
rstone edited the test plan for this revision. (Show Details)
rstone added a reviewer: jfv.
rstone added subscribers: pjd, Unknown Object (MLST).

What testing has been done with this change?

jfv edited edge metadata.
This revision is now accepted and ready to land.Feb 18 2015, 10:30 PM

The primary testing that I did was to ensure that a kernel with "device zfs" would still link. However, I suppose that doesn't exclude the possibility of their being an zfs source file that is built wrong and doesn't include this header. Is there a zfs test suite that I could run?

In D1881#9, @rstone wrote:

The primary testing that I did was to ensure that a kernel with "device zfs" would still link. However, I suppose that doesn't exclude the possibility of their being an zfs source file that is built wrong and doesn't include this header. Is there a zfs test suite that I could run?

Not yet, unfortunately. It's in the zfsd project branch, but needs some work so it can be pushed into head.

It'd probably be sufficient to load zfs and create pools/filesystems, and a few other basic tests. Are the userland bits covered (and tested) with this change? This is a <sys/nvpair.h> overload (effectively), which may be pulled into the userland zfs builds.

Ok, I did a full buildworld/buildkernel (with device zfs in my kernel config) and installed it into a couple of zfs. I tested creating a zpool out of a mfs disk, creating filesystems, setting some properties, doing a git clone into a file system, and then doing a zfs send of the result from one vm to the problem. No problems cropped up.

Err, that was supposed to be "installed it into a couple of *VMs*"

will added a reviewer: will.

Ok, LGTM. Thanks for your effort!

This has already been committed, so I've missed the train, but I still would like to comment. And I hope that my comment won't be just a rant.
So, I don't like this change for several reasons.

For one, given that nv(9) is a new thing while libnvpair already existed it would have been wise to select non-conflicting names for the new interfaces. And I think that it's still not too late to do that.
Then, it seems that this change has solved a theoretical problem as I don't think that currently there is any executable that links to both libraries.Perhaps I am wrong.
Also, libnvpair.so was treated like a "private" library: its ABI was wildly changed but its version hasn't been bumped.
Finally, from ABI point of view libnvpair now appears very inconsistent: many of its interfaces are prefixed with "illumos_" while quite a few don't have that prefix.

Now, why am I interested in libnvpair?
At work we have a Python module that interfaces libzfs_core and by necessity libnvpair through CFFI.
The module used to work perfectly well across FreeBSD, illumos and Linux because the library interfaces on ABI level are the same (from CFFI's point of view) across platforms.
After this change FreeBSD is an odd platform. I have to add a workaround to keep the module working. And even the workaround is not trivial because of the mix of prefixed and non-prefixed names.
Hope you sympathize.

Another thought.

Or was the intention only to make nv(9) and hrm.. nvpair compatible only within the kernel? Section 9 seems like a big hint here, but I am not sure.
If the intention was such, then my previous comment is to inform that that change affected the userland as well.