undocumented libzfs API changes broke "zfs list"

While OpenZFS does permit breaking changes to the libzfs API, we should
avoid these changes when reasonably possible, and take steps to mitigate
the impact to consumers when changes are necessary.

Commit e4288a8397 made a libzfs API change that is especially
difficult for consumers because there is no change to the function
signatures, only to their behavior.  Therefore, consumers can't notice
that there was a change at compile time.  Also, the API change was
incompletely and incorrectly documented.

The commit message mentions `zfs_get_prop()` [sic], but all callers of
`get_numeric_property()` are impacted: `zfs_prop_get()`,
`zfs_prop_get_numeric()`, and `zfs_prop_get_int()`.

`zfs_prop_get_int()` always calls `get_numeric_property(src=NULL)`, so
it assumes that the filesystem is not mounted.  This means that e.g.
`zfs_prop_get_int(ZFS_PROP_MOUNTED)` always returns 0.

The documentation says that to preserve the previous behavior, callers
should initialize `*src=ZPROP_SRC_NONE`, and some callers were changed
to do that.  However, the existing behavior is actually preserved by
initializing `*src=ZPROP_SRC_ALL`, not `NONE`.

The code comment above `zfs_prop_get()` says, "src: ... NULL will be
treated as ZPROP_SRC_ALL.".  However, the code actually treats NULL as
ZPROP_SRC_NONE.  i.e. `zfs_prop_get(src=NULL)` assumes that the
filesystem is not mounted.

There are several existing calls which use `src=NULL` which are impacted
by the API change, most noticeably those used by `zfs list`, which now
assumes that filesystems are not mounted.  For example,
`zfs list -o name,mounted` previously indicated whether a filesystem was
mounted or not, but now it always (incorrectly) indicates that the
filesystem is not mounted (`MOUNTED: no`).  Similarly, properties that
are set at mount time are ignored.  E.g. `zfs list -o name,atime` may
display an incorrect value if it was set at mount time.

To address these problems, this commit reverts commit e4288a8397bb1f:
"zfs get: don't lookup mount options when using "-s local""

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes #11999
This commit is contained in:
Matthew Ahrens 2021-05-06 11:24:56 -07:00 committed by GitHub
parent b1e44cdcea
commit 610cb4fb8c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 13 additions and 18 deletions

View File

@ -1882,6 +1882,7 @@ get_callback(zfs_handle_t *zhp, void *data)
{ {
char buf[ZFS_MAXPROPLEN]; char buf[ZFS_MAXPROPLEN];
char rbuf[ZFS_MAXPROPLEN]; char rbuf[ZFS_MAXPROPLEN];
zprop_source_t sourcetype;
char source[ZFS_MAX_DATASET_NAME_LEN]; char source[ZFS_MAX_DATASET_NAME_LEN];
zprop_get_cbdata_t *cbp = data; zprop_get_cbdata_t *cbp = data;
nvlist_t *user_props = zfs_get_user_props(zhp); nvlist_t *user_props = zfs_get_user_props(zhp);
@ -1892,7 +1893,6 @@ get_callback(zfs_handle_t *zhp, void *data)
boolean_t received = is_recvd_column(cbp); boolean_t received = is_recvd_column(cbp);
for (; pl != NULL; pl = pl->pl_next) { for (; pl != NULL; pl = pl->pl_next) {
zprop_source_t sourcetype = cbp->cb_sources;
char *recvdval = NULL; char *recvdval = NULL;
/* /*
* Skip the special fake placeholder. This will also skip over * Skip the special fake placeholder. This will also skip over
@ -4660,7 +4660,7 @@ zfs_do_send(int argc, char **argv)
*/ */
if (fromname && (cp = strchr(fromname, '@')) != NULL) { if (fromname && (cp = strchr(fromname, '@')) != NULL) {
char origin[ZFS_MAX_DATASET_NAME_LEN]; char origin[ZFS_MAX_DATASET_NAME_LEN];
zprop_source_t src = ZPROP_SRC_NONE; zprop_source_t src;
(void) zfs_prop_get(zhp, ZFS_PROP_ORIGIN, (void) zfs_prop_get(zhp, ZFS_PROP_ORIGIN,
origin, sizeof (origin), &src, NULL, 0, B_FALSE); origin, sizeof (origin), &src, NULL, 0, B_FALSE);

View File

@ -2180,8 +2180,7 @@ get_numeric_property(zfs_handle_t *zhp, zfs_prop_t prop, zprop_source_t *src,
* its presence. * its presence.
*/ */
if (!zhp->zfs_mntcheck && if (!zhp->zfs_mntcheck &&
(mntopt_on != NULL || prop == ZFS_PROP_MOUNTED) && (mntopt_on != NULL || prop == ZFS_PROP_MOUNTED)) {
(src && (*src & ZPROP_SRC_TEMPORARY))) {
libzfs_handle_t *hdl = zhp->zfs_hdl; libzfs_handle_t *hdl = zhp->zfs_hdl;
struct mnttab entry; struct mnttab entry;
@ -2596,16 +2595,9 @@ zcp_check(zfs_handle_t *zhp, zfs_prop_t prop, uint64_t intval,
} }
/* /*
* Retrieve a property from the given object. * Retrieve a property from the given object. If 'literal' is specified, then
* * numbers are left as exact values. Otherwise, numbers are converted to a
* Arguments: * human-readable form.
* src : On call, this must contain the bitmap of ZPROP_SRC_* types to
* query. Properties whose values come from a different source
* may not be returned. NULL will be treated as ZPROP_SRC_ALL. On
* return, if not NULL, this variable will contain the source for
* the queried property.
* literal : If specified, then numbers are left as exact values. Otherwise,
* they are converted to a human-readable form.
* *
* Returns 0 on success, or -1 on error. * Returns 0 on success, or -1 on error.
*/ */
@ -2628,6 +2620,9 @@ zfs_prop_get(zfs_handle_t *zhp, zfs_prop_t prop, char *propbuf, size_t proplen,
if (received && zfs_prop_readonly(prop)) if (received && zfs_prop_readonly(prop))
return (-1); return (-1);
if (src)
*src = ZPROP_SRC_NONE;
switch (prop) { switch (prop) {
case ZFS_PROP_CREATION: case ZFS_PROP_CREATION:
/* /*

View File

@ -575,7 +575,7 @@ get_snapshot_names(differ_info_t *di, const char *fromsnap,
* tosnap is a clone of a fromsnap descendant. * tosnap is a clone of a fromsnap descendant.
*/ */
char origin[ZFS_MAX_DATASET_NAME_LEN]; char origin[ZFS_MAX_DATASET_NAME_LEN];
zprop_source_t src = ZPROP_SRC_NONE; zprop_source_t src;
zfs_handle_t *zhp; zfs_handle_t *zhp;
di->ds = zfs_alloc(di->zhp->zfs_hdl, tdslen + 1); di->ds = zfs_alloc(di->zhp->zfs_hdl, tdslen + 1);

View File

@ -266,7 +266,7 @@ zfs_is_mountable(zfs_handle_t *zhp, char *buf, size_t buflen,
zprop_source_t *source, int flags) zprop_source_t *source, int flags)
{ {
char sourceloc[MAXNAMELEN]; char sourceloc[MAXNAMELEN];
zprop_source_t sourcetype = ZPROP_SRC_NONE; zprop_source_t sourcetype;
if (!zfs_prop_valid_for_type(ZFS_PROP_MOUNTPOINT, zhp->zfs_type, if (!zfs_prop_valid_for_type(ZFS_PROP_MOUNTPOINT, zhp->zfs_type,
B_FALSE)) B_FALSE))
@ -765,7 +765,7 @@ zfs_share_proto(zfs_handle_t *zhp, zfs_share_proto_t *proto)
char shareopts[ZFS_MAXPROPLEN]; char shareopts[ZFS_MAXPROPLEN];
char sourcestr[ZFS_MAXPROPLEN]; char sourcestr[ZFS_MAXPROPLEN];
zfs_share_proto_t *curr_proto; zfs_share_proto_t *curr_proto;
zprop_source_t sourcetype = ZPROP_SRC_NONE; zprop_source_t sourcetype;
int err = 0; int err = 0;
if (!zfs_is_mountable(zhp, mountpoint, sizeof (mountpoint), NULL, 0)) if (!zfs_is_mountable(zhp, mountpoint, sizeof (mountpoint), NULL, 0))

View File

@ -2662,7 +2662,7 @@ static zfs_handle_t *
recv_open_grand_origin(zfs_handle_t *zhp) recv_open_grand_origin(zfs_handle_t *zhp)
{ {
char origin[ZFS_MAX_DATASET_NAME_LEN]; char origin[ZFS_MAX_DATASET_NAME_LEN];
zprop_source_t src = ZPROP_SRC_NONE; zprop_source_t src;
zfs_handle_t *ozhp = zfs_handle_dup(zhp); zfs_handle_t *ozhp = zfs_handle_dup(zhp);
while (ozhp != NULL) { while (ozhp != NULL) {