From 43fd8e6d5ec2889cbe60f153fad3a9749b42f12f Mon Sep 17 00:00:00 2001 From: Andriy Gapon Date: Fri, 14 Apr 2017 18:14:02 +0000 Subject: [PATCH] 7745 print error if lzc_* is called before libzfs_core_init illumos/illumos-gate@7c13517fff71be473e47531ef4330160c042bedc https://github.com/illumos/illumos-gate/commit/7c13517fff71be473e47531ef4330160c042bedc https://www.illumos.org/issues/7745 The problem is that consumers of `libZFS_Core` that forget to call `libzfs_core_init()` before calling any other function of the library are having a hard time realizing their mistake. The library's internal file descriptor is declared as global static, which is ok, but it is not initialized explicitly; therefore, it defaults to 0, which is a valid file descriptor. If `libzfs_core_init()`, which explicitly initializes the correct fd, is skipped, the ioctl functions return errors that do not have anything to do with `libZFS_Core`, where the problem is actually located. Even though assertions for that existed within `libZFS_Core` for debug builds, they were never enabled because the `-DDEBUG` flag was missing from the compiler flags. This patch applies the following changes: 1. It adds `-DDEBUG` for debug builds of `libZFS_Core` and `libzfs`, to enable their assertions on debug builds. 2. It corrects an assertion within `libzfs`, where a function had been spelled incorrectly (`zpool_prop_unsupported()`) and nobody knew because the `-DDEBUG` flag was missing, and the preprocessor was taking that part of the code away. 3. The library's internal fd is initialized to `-1` and `VERIFY` assertions have been placed to check that the fd is not equal to `-1` before issuing any ioctl. It is important here to note, that the `VERIFY` assertions exist in both debug and non-debug builds. 4. In `libzfs_core_fini` we make sure to never increment the refcount of our fd below 0, and also reset the fd to `-1` when no one refers to it. The reason for this, is for the rare case that the consumer closes all references but then calls one of the library's functions without using `libzfs_core_init()` first, and in the mean time, a previous call to `open()` decided to reuse our previous fd. This scenario would have passed our assertion in Reviewed by: Pavel Zakharov Reviewed by: Matthew Ahrens Approved by: Dan McDonald Author: Serapheim Dimitropoulos --- lib/libzfs/common/libzfs_pool.c | 2 +- lib/libzfs/common/libzfs_sendrecv.c | 9 +++++++++ lib/libzfs_core/common/libzfs_core.c | 16 +++++++++++++--- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/lib/libzfs/common/libzfs_pool.c b/lib/libzfs/common/libzfs_pool.c index 1808de2387c7..fe7d68be18cf 100644 --- a/lib/libzfs/common/libzfs_pool.c +++ b/lib/libzfs/common/libzfs_pool.c @@ -818,7 +818,7 @@ zpool_prop_get_feature(zpool_handle_t *zhp, const char *propname, char *buf, const char *feature = strchr(propname, '@') + 1; supported = zpool_prop_feature(propname); - ASSERT(supported || zfs_prop_unsupported(propname)); + ASSERT(supported || zpool_prop_unsupported(propname)); /* * Convert from feature name to feature guid. This conversion is diff --git a/lib/libzfs/common/libzfs_sendrecv.c b/lib/libzfs/common/libzfs_sendrecv.c index a1544206a3f5..82baeca9373c 100644 --- a/lib/libzfs/common/libzfs_sendrecv.c +++ b/lib/libzfs/common/libzfs_sendrecv.c @@ -266,6 +266,15 @@ cksummer(void *arg) ofp = fdopen(dda->inputfd, "r"); while (ssread(drr, sizeof (*drr), ofp) != 0) { + /* + * kernel filled in checksum, we are going to write same + * record, but need to regenerate checksum. + */ + if (drr->drr_type != DRR_BEGIN) { + bzero(&drr->drr_u.drr_checksum.drr_checksum, + sizeof (drr->drr_u.drr_checksum.drr_checksum)); + } + switch (drr->drr_type) { case DRR_BEGIN: { diff --git a/lib/libzfs_core/common/libzfs_core.c b/lib/libzfs_core/common/libzfs_core.c index 7e7891798d04..9868d70ffb5d 100644 --- a/lib/libzfs_core/common/libzfs_core.c +++ b/lib/libzfs_core/common/libzfs_core.c @@ -85,7 +85,7 @@ #include #include -static int g_fd; +static int g_fd = -1; static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER; static int g_refcount; @@ -110,9 +110,14 @@ libzfs_core_fini(void) { (void) pthread_mutex_lock(&g_lock); ASSERT3S(g_refcount, >, 0); - g_refcount--; - if (g_refcount == 0) + + if (g_refcount > 0) + g_refcount--; + + if (g_refcount == 0 && g_fd != -1) { (void) close(g_fd); + g_fd = -1; + } (void) pthread_mutex_unlock(&g_lock); } @@ -126,6 +131,7 @@ lzc_ioctl(zfs_ioc_t ioc, const char *name, size_t size; ASSERT3S(g_refcount, >, 0); + VERIFY3S(g_fd, !=, -1); (void) strlcpy(zc.zc_name, name, sizeof (zc.zc_name)); @@ -328,6 +334,9 @@ lzc_exists(const char *dataset) */ zfs_cmd_t zc = { 0 }; + ASSERT3S(g_refcount, >, 0); + VERIFY3S(g_fd, !=, -1); + (void) strlcpy(zc.zc_name, dataset, sizeof (zc.zc_name)); return (ioctl(g_fd, ZFS_IOC_OBJSET_STATS, &zc) == 0); } @@ -573,6 +582,7 @@ recv_impl(const char *snapname, nvlist_t *props, const char *origin, int error; ASSERT3S(g_refcount, >, 0); + VERIFY3S(g_fd, !=, -1); /* zc_name is name of containing filesystem */ (void) strlcpy(zc.zc_name, snapname, sizeof (zc.zc_name));