Skip to content
This repository has been archived by the owner on Jul 25, 2024. It is now read-only.

ZoL master and FreeBSD 11.2 support #64

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dsheets
Copy link

@dsheets dsheets commented Jul 15, 2018

I've updated the API to support libzfs as of openzfs/zfs@2e5dc44 (current master) which happens to coincide with the libzfs API as shipped in FreeBSD 11.2. I used Debian GNU/Linux 9 with kernel 4.9.88-1+deb9u1 and ZoL master to test on Linux. I generalized the Linux support in the build system to use the include paths in the pkg-config output and output from a small shell script which queries gcc to find the system include directories.

I haven't put a regenerated libzfs-sys/src/bindings.rs in this PR as my generated bindings don't seem to pretty-print and I haven't investigated why not.

A single code location (libzfs-sys/build.rs:115) presupposes the path to the ZFS source on Linux and assumes that you are running against the master of ZoL. Maybe this should be configurable via an environment variable or cargo parameter or similar.

Speaking of environment variables, I could not figure out how env::set_var("LIBCLANG_PATH", "/opt/llvm-5.0.0/lib64/"); affects anything after reading https://github.com/KyleMayes/clang-sys/blob/67f7d8c25eff694d7ba58ff42da6e5b502413b7d/README.md and related source which only uses LIBCLANG_PATH at clang-sys build time.

The line fn list_gcc_include_paths() -> impl Iterator<Item = PathBuf> { uses a rust 1.26+ feature. I'm not sure what the previously preferred solution is to this usage if compatibility for rust < 1.26 is desired.

Probably the most confusing issue I had was with boolean/boolean_t. Use of boolean only seems to work if the declaration is typedef enum boolean { ... } boolean_t but I could only find typedef enum { ... } boolean_t forms in the version history. Using boolean_t is only slightly longer and appears to work universally after NEED_SOLARIS_BOOLEAN is declared for FreeBSD.

Closes #63.

The LIBCLANG_PATH environment variable was unused, the 'boolean' enum is
anonymous and typedef'd to 'boolean_t', the ZoL-specific 'thread_init' and
'thread_fini' have disappeared, gcc is now queried for system include paths,
and the include paths of the libzfs pkg-config are now used.

Signed-off-by: David Sheets <[email protected]>
ZoL `master` (2e5dc449c1a65e0b0bf730fd69c9b5804bd57ee8) and FreeBSD 11.2 have
compatible libzfs APIs but FreeBSD lacks pkg-config and has different ZFS source
include directories. Fixes whamcloud#63.

Signed-off-by: David Sheets <[email protected]>
@dsheets
Copy link
Author

dsheets commented Jul 15, 2018

It looks like LIBCLANG_PATH is somehow used and necessary for the CI instances to find their libclang install. I've added it back with a comment about this.

Unfortunately, the latest release of ZoL and the release of ZFS in FreeBSD 11.2 are not compatible. I'm not sure how you'd like to proceed with ZoL version dependency. Due to this and the hard-coded ZoL source path, the CI will always fail on this patchset due to 'missing' headers.

bindings.clang_arg(flag)
})
// include directory for zfsonlinux/zfs master branch
.clang_arg("-I/usr/src/zfs-0.7.0/include")
Copy link
Member

@jgrund jgrund Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will get a bit farther if it is changed to:

.clang_arg("-I/usr/src/zfs-0.7.9/include")

@jgrund
Copy link
Member

jgrund commented Jul 16, 2018

Unfortunately, the latest release of ZoL and the release of ZFS in FreeBSD 11.2 are not compatible.

What incompatibilities are you seeing?

@jgrund
Copy link
Member

jgrund commented Jul 16, 2018

A single code location (libzfs-sys/build.rs:115) presupposes the path to the ZFS source on Linux and assumes that you are running against the master of ZoL. Maybe this should be configurable via an environment variable or cargo parameter or similar.

Yes, we should probably be a bit more generic about how we build now that we want to support multiple OS / releases

@jgrund
Copy link
Member

jgrund commented Jul 16, 2018

Thanks for doing this @dsheets

For reference, I've tested quickly on CentOS 7, using latest ZOL 0.7.9 RPM and got following issues when compiling:

error[E0531]: cannot find unit struct/variant or constant `zfs_prop_t_ZPROP_INVAL` in module `sys`
  --> libzfs/src/zfs.rs:65:18
   |
65 |             sys::zfs_prop_t_ZPROP_INVAL => self.user_props()
   |                  ^^^^^^^^^^^^^^^^^^^^^^ did you mean `zfs_prop_t_ZFS_PROP_BAD`?

error[E0599]: no associated item named `B_TRUE` found for type `u32` in the current scope
  --> libzfs/src/zfs.rs:50:17
   |
50 |                 sys::boolean_t::B_TRUE,
   |                 ^^^^^^^^^^^^^^^^^^^^^^ associated item not found in `u32`

error[E0599]: no associated item named `B_TRUE` found for type `u32` in the current scope
  --> libzfs/src/zfs.rs:51:17
   |
51 |                 sys::boolean_t::B_TRUE,
   |                 ^^^^^^^^^^^^^^^^^^^^^^ associated item not found in `u32`

error[E0599]: no associated item named `B_TRUE` found for type `u32` in the current scope
  --> libzfs/src/zfs.rs:85:25
   |
85 |                         sys::boolean_t::B_TRUE,
   |                         ^^^^^^^^^^^^^^^^^^^^^^ associated item not found in `u32`

error[E0599]: no associated item named `B_FALSE` found for type `u32` in the current scope
  --> libzfs/src/zpool.rs:59:17
   |
59 |                 sys::boolean_t::B_FALSE,
   |                 ^^^^^^^^^^^^^^^^^^^^^^^ associated item not found in `u32`

error[E0599]: no associated item named `B_FALSE` found for type `u32` in the current scope
   --> libzfs/src/zpool.rs:144:67
    |
144 |         let code = unsafe { sys::zpool_disable_datasets(self.raw, sys::boolean_t::B_FALSE) };
    |                                                                   ^^^^^^^^^^^^^^^^^^^^^^^ associated item not found in `u32`

error[E0599]: no associated item named `B_FALSE` found for type `u32` in the current scope
   --> libzfs/src/zpool.rs:152:57
    |
152 |         let code = unsafe { sys::zpool_export(self.raw, sys::boolean_t::B_FALSE, ptr::null_mut()) };
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^ associated item not found in `u32`

With binding changes, boolean is now generated for me as:

pub const boolean_B_FALSE: boolean = 0;
pub const boolean_B_TRUE: boolean = 1;
pub type boolean = u32;
pub use self::boolean as boolean_t;

Which seems as though there is no linkage between boolean type and B_TRUE / B_FALSE.

Previously, the generation was:

pub mod boolean {
    pub type Type = u32;
    pub const B_FALSE: Type = 0;
    pub const B_TRUE: Type = 1;
}
pub use self::boolean::Type as boolean_t;

FWIW, ZOL boolean def is here: https://github.com/zfsonlinux/zfs/blob/2e5dc449c1a65e0b0bf730fd69c9b5804bd57ee8/tests/zfs-tests/tests/functional/checksum/edonr_test.c#L42

sys::zfs_prop_t_ZPROP_INVAL seems to be a newer addition post 0.7.9.

It seems like we will need to account for OS, binding differences. I guess this could be done either by shipping with a library of bindings and selecting the correct one at runtime, or by being more generic and accounting for more build paths when a user downloads the crate.

@fabianfreyer
Copy link

If we go the second way pointed out by @jgrund:

It seems like we will need to account for OS, binding differences. I guess this could be done [...] by being more generic and accounting for more build paths when a user downloads the crate.

Something that could be done here is generating separate bindings for each libzfs version and make them modules behind feature flags (e.g. zol-0.7.9 or fbsd-11.2). The actual version could then be probed in build.rs and exposed with rust-lang/cargo#1505.

The obvious disadvantage here would be that the configuration flags exposed by build.rs are local to the crate, which would mean that libzfs-sys as well as the compatibility wrapper around it would have to be in the same crate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants