socsvn commit: r324821 - soc2017/kneitinger/libbe-head/lib/libbe

kneitinger at FreeBSD.org kneitinger at FreeBSD.org
Wed Jul 19 12:00:54 UTC 2017


Author: kneitinger
Date: Wed Jul 19 12:00:51 2017
New Revision: 324821
URL: http://svnweb.FreeBSD.org/socsvn/?view=rev&rev=324821

Log:
  libbe(3): refactor library to avoid redundant error checking
  

Modified:
  soc2017/kneitinger/libbe-head/lib/libbe/be.c
  soc2017/kneitinger/libbe-head/lib/libbe/be.h
  soc2017/kneitinger/libbe-head/lib/libbe/be_access.c
  soc2017/kneitinger/libbe-head/lib/libbe/be_impl.h
  soc2017/kneitinger/libbe-head/lib/libbe/be_info.c

Modified: soc2017/kneitinger/libbe-head/lib/libbe/be.c
==============================================================================
--- soc2017/kneitinger/libbe-head/lib/libbe/be.c	Wed Jul 19 09:59:32 2017	(r324820)
+++ soc2017/kneitinger/libbe-head/lib/libbe/be.c	Wed Jul 19 12:00:51 2017	(r324821)
@@ -26,6 +26,7 @@
  * SUCH DAMAGE.
  */
 
+#include <sys/stat.h>
 #include <sys/types.h>
 
 #include <ctype.h>
@@ -44,60 +45,61 @@
 libbe_handle_t *
 libbe_init(void)
 {
+	char buf[BE_MAXPATHLEN];
+	struct stat sb;
+	dev_t root_dev;
 	libbe_handle_t *lbh;
-	char buf[MAXPATHLEN];
+	char *pos;
 
+	// TODO: use errno here??
 
-	if ((lbh = calloc(1, sizeof(libbe_handle_t))) == NULL) {
+	/* Verify that /boot and / are mounted on the same filesystem */
+	if (stat("/", &sb) != 0) {
 		return (NULL);
 	}
+	root_dev = sb.st_dev;
 
-	if ((lbh->lzh = libzfs_init()) == NULL) {
-		free(lbh);
+	if (stat("/boot", &sb) != 0) {
 		return (NULL);
 	}
 
-	/* Obtain path to active boot environment */
-	if ((kenv(KENV_GET, "zfs_be_active", buf, MAXPATHLEN)) == -1) {
-		libzfs_fini(lbh->lzh);
-		free(lbh);
+	if (root_dev != sb.st_dev) {
 		return (NULL);
 	}
 
-	/* Remove leading 'zfs:' if present, otherwise use value as-is */
-	char *path = strrchr(buf, ':');
-	path = path ? path + 1 : buf;
+	if ((lbh = calloc(1, sizeof(libbe_handle_t))) == NULL) {
+		return (NULL);
+	}
 
-	/* Obtain zfs_handle_t to the active bootenv */
-	if ((lbh->be_active = zfs_open(lbh->lzh, path,
-	    ZFS_TYPE_DATASET)) == NULL) {
-		libzfs_fini(lbh->lzh);
+	if ((lbh->lzh = libzfs_init()) == NULL) {
 		free(lbh);
 		return (NULL);
 	}
 
-	/* Obtain path to boot environment root */
-	if ((kenv(KENV_GET, "zfs_be_root", buf, MAXPATHLEN)) == -1) {
-		zfs_close(lbh->be_active);
+	/* Obtain path to active boot environment */
+	if ((kenv(KENV_GET, "zfs_be_active", lbh->active,
+	    BE_MAXPATHLEN)) == -1) {
 		libzfs_fini(lbh->lzh);
 		free(lbh);
 		return (NULL);
 	}
 
+	/* Remove leading 'zfs:' if present, otherwise use value as-is */
+	if ((pos = strrchr(lbh->active, ':')) != NULL) {
+		strncpy(lbh->active, pos + sizeof(char), BE_MAXPATHLEN);
+	}
 
-	/* Obtain zfs_handle_t to the bootenv root */
-	if ((lbh->be_root =
-	    zfs_open(lbh->lzh, buf, ZFS_TYPE_DATASET)) == NULL) {
-		zfs_close(lbh->be_active);
+	/* Obtain path to boot environment root */
+	if ((kenv(KENV_GET, "zfs_be_root", lbh->root, BE_MAXPATHLEN)) == -1) {
 		libzfs_fini(lbh->lzh);
 		free(lbh);
 		return (NULL);
 	}
 
-	/* TODO: verify that /boot is mounted on the active be */
-
-	prop_list_builder(lbh);
-
+	/* Remove leading 'zfs:' if present, otherwise use value as-is */
+	if ((pos = strrchr(lbh->root, ':')) != NULL) {
+		strncpy(lbh->root, pos + sizeof(char), BE_MAXPATHLEN);
+	}
 
 	return (lbh);
 }
@@ -109,10 +111,7 @@
 void
 libbe_close(libbe_handle_t *lbh)
 {
-	zfs_close(lbh->be_active);
-	zfs_close(lbh->be_root);
 	libzfs_fini(lbh->lzh);
-	prop_list_free(lbh);
 	free(lbh);
 }
 
@@ -126,14 +125,14 @@
 be_destroy(libbe_handle_t *lbh, char *name, int options)
 {
 	zfs_handle_t *fs;
-	char path[MAXPATHLEN];
+	char path[BE_MAXPATHLEN];
 	char *p = path;
 	int mounted;
 	int force = options & BE_DESTROY_FORCE;
 
 	int err = BE_ERR_SUCCESS;
 
-	snprintf(path, MAXPATHLEN, "%s/%s", zfs_get_name(lbh->be_root), name);
+	snprintf(path, BE_MAXPATHLEN, "%s/%s", lbh->root, name);
 
 	if (!zfs_dataset_exists(lbh->lzh, path, ZFS_TYPE_DATASET)) {
 		return (set_error(lbh, BE_ERR_NOENT));
@@ -141,7 +140,7 @@
 
 	fs = zfs_open(lbh->lzh, p, ZFS_TYPE_DATASET);
 
-	if (strcmp(path, zfs_get_name(lbh->be_active)) != 0) {
+	if (strcmp(path, lbh->active) != 0) {
 		return (set_error(lbh, BE_ERR_DESTROYACT));
 	}
 
@@ -162,8 +161,8 @@
 be_create(libbe_handle_t *lbh, char *name)
 {
 	int err, pos;
-	char be_path[MAXPATHLEN];
-	char snap_name[MAXPATHLEN];
+	char be_path[BE_MAXPATHLEN];
+	char snap_name[BE_MAXPATHLEN];
 	time_t rawtime;
 	zfs_handle_t *snap_hdl;
 
@@ -176,9 +175,11 @@
 		return (err);
 	}
 
-	pos = snprintf(be_path, MAXPATHLEN, "%s/%s", be_root_path(lbh), name);
+	// TODO: be_root_concat
+	pos =
+	    snprintf(be_path, BE_MAXPATHLEN, "%s/%s", be_root_path(lbh), name);
 
-	if ((pos < 0) || (pos >= MAXPATHLEN)) {
+	if ((pos < 0) || (pos >= BE_MAXPATHLEN)) {
 		err = set_error(lbh, BE_ERR_PATHLEN);
 	}
 
@@ -189,8 +190,8 @@
 
 
 	time(&rawtime);
-	pos = snprintf(snap_name, MAXPATHLEN, "%s", be_active_path(lbh));
-	strftime(snap_name + pos, MAXPATHLEN - pos,
+	pos = snprintf(snap_name, BE_MAXPATHLEN, "%s", be_active_path(lbh));
+	strftime(snap_name + pos, BE_MAXPATHLEN - pos,
 	    "@%F-%T", localtime(&rawtime));
 
 
@@ -200,6 +201,7 @@
 		return (-1);
 	}
 
+	// TODO: verify not null!!
 	snap_hdl = zfs_open(lbh->lzh, snap_name, ZFS_TYPE_SNAPSHOT);
 
 	// TODO: should any props be in the last arg (nvlist)?
@@ -219,8 +221,8 @@
 be_create_from_existing(libbe_handle_t *lbh, char *name, char *snap)
 {
 	int err, pos;
-	char be_path[MAXPATHLEN];
-	char snap_name[MAXPATHLEN];
+	char be_path[BE_MAXPATHLEN];
+	char snap_name[BE_MAXPATHLEN];
 	time_t rawtime;
 	zfs_handle_t *snap_hdl;
 
@@ -242,9 +244,10 @@
 		return (err);
 	}
 
-	pos = snprintf(be_path, MAXPATHLEN, "%s/%s", be_root_path(lbh), name);
+	pos =
+	    snprintf(be_path, BE_MAXPATHLEN, "%s/%s", be_root_path(lbh), name);
 
-	if ((pos < 0) || (pos >= MAXPATHLEN)) {
+	if ((pos < 0) || (pos >= BE_MAXPATHLEN)) {
 		err = set_error(lbh, BE_ERR_PATHLEN);
 	}
 
@@ -276,12 +279,12 @@
 be_validate_snap(libbe_handle_t *lbh, char *snap_name)
 {
 	zfs_handle_t *zfs_hdl;
-	char buf[MAXPATHLEN];
+	char buf[BE_MAXPATHLEN];
 	char *snap_delim;
 	char *mountpoint;
 	int err = 0;
 
-	if (strlen(snap_name) >= MAXPATHLEN) {
+	if (strlen(snap_name) >= BE_MAXPATHLEN) {
 		return (BE_ERR_PATHLEN);
 	}
 
@@ -289,7 +292,7 @@
 		return (BE_ERR_NOENT);
 	}
 
-	strncpy(buf, snap_name, MAXPATHLEN);
+	strncpy(buf, snap_name, BE_MAXPATHLEN);
 
 	if ((snap_delim = strchr(buf, '@')) == NULL) {
 		return (BE_ERR_INVALIDNAME);
@@ -301,12 +304,12 @@
 		return (BE_ERR_NOORIGIN);
 	}
 
-	if (err = zfs_prop_get(zfs_hdl, ZFS_PROP_MOUNTPOINT, buf, MAXPATHLEN,
+	if (err = zfs_prop_get(zfs_hdl, ZFS_PROP_MOUNTPOINT, buf, BE_MAXPATHLEN,
 	    NULL, NULL, 0, 1)) {
 		err = BE_ERR_INVORIGIN;
 	}
 
-	if ((err != 0) && (strncmp(buf, "/", MAXPATHLEN) != 0)) {
+	if ((err != 0) && (strncmp(buf, "/", BE_MAXPATHLEN) != 0)) {
 		err = BE_ERR_INVORIGIN;
 	}
 
@@ -317,21 +320,35 @@
 
 
 /*
- * Appends the name argument on the root boot environment path and copies the
- * resulting string into the result buffer. Returns 0 upon success and
- * BE_ERR_PATHLEN if the resulting path is longer than MAXPATHLEN. Does not set
- * internal library error state.
+ * Idempotently appends the name argument to the root boot environment path
+ * and copies the resulting string into the result buffer (which is assumed
+ * to be at least BE_MAXPATHLEN characters long. Returns BE_ERR_SUCCESS upon
+ * success, BE_ERR_PATHLEN if the resulting path is longer than BE_MAXPATHLEN,
+ * or BE_ERR_INVALIDNAME if the name is a path that does not begin with
+ * zfs_be_root. Does not set internal library error state.
  */
 int
 be_root_concat(libbe_handle_t *lbh, char *name, char *result)
 {
-	// TODO: make idempotent. As in: if it already is of the form:
-	// pool/beroot/be, return that
+	size_t name_len, root_len;
+
+	name_len = strlen(name);
+
+	/* Act idempotently; return be name if it is already a full path */
+	if (strrchr(name, '/') != NULL) {
+		if (strstr(name, lbh->root) != name) {
+			return (BE_ERR_INVALIDNAME);
+		}
 
-	const char *root = be_root_path(lbh);
-	int n = snprintf(result, MAXPATHLEN, "%s/%s", root, name);
+		if (name_len >= BE_MAXPATHLEN) {
+			return (BE_ERR_PATHLEN);
+		}
 
-	if ((n >= 0) && (n < MAXPATHLEN)) {
+		strncpy(result, name, BE_MAXPATHLEN);
+		return (BE_ERR_SUCCESS);
+	} else if (name_len + root_len + 1 < BE_MAXPATHLEN) {
+		root_len = strlen(lbh->root);
+		snprintf(result, BE_MAXPATHLEN, "%s/%s", lbh->root, name);
 		return (BE_ERR_SUCCESS);
 	}
 
@@ -367,8 +384,8 @@
 int
 be_rename(libbe_handle_t *lbh, char *old, char *new)
 {
-	char full_old[MAXPATHLEN];
-	char full_new[MAXPATHLEN];
+	char full_old[BE_MAXPATHLEN];
+	char full_new[BE_MAXPATHLEN];
 	zfs_handle_t *zfs_hdl;
 	int err;
 

Modified: soc2017/kneitinger/libbe-head/lib/libbe/be.h
==============================================================================
--- soc2017/kneitinger/libbe-head/lib/libbe/be.h	Wed Jul 19 09:59:32 2017	(r324820)
+++ soc2017/kneitinger/libbe-head/lib/libbe/be.h	Wed Jul 19 12:00:51 2017	(r324821)
@@ -32,6 +32,8 @@
 #include <libzfs.h>
 #include <stdbool.h>
 
+#define BE_MAXPATHLEN    ZFS_MAX_DATASET_NAME_LEN
+
 typedef struct libbe_handle libbe_handle_t;
 
 typedef enum be_error {

Modified: soc2017/kneitinger/libbe-head/lib/libbe/be_access.c
==============================================================================
--- soc2017/kneitinger/libbe-head/lib/libbe/be_access.c	Wed Jul 19 09:59:32 2017	(r324820)
+++ soc2017/kneitinger/libbe-head/lib/libbe/be_access.c	Wed Jul 19 12:00:51 2017	(r324821)
@@ -35,7 +35,7 @@
 int
 be_mount(libbe_handle_t *lbh, char *bootenv, char *mountpoint, int flags)
 {
-	char be[MAXPATHLEN];
+	char be[BE_MAXPATHLEN];
 	zfs_handle_t *zfs_hdl;
 	char *path;
 	int mntflags;
@@ -75,7 +75,7 @@
 be_unmount(libbe_handle_t *lbh, char *bootenv, int flags)
 {
 	int err, mntflags;
-	char be[MAXPATHLEN];
+	char be[BE_MAXPATHLEN];
 	struct statfs *mntbuf;
 	int mntsize;
 	char *mntpath;

Modified: soc2017/kneitinger/libbe-head/lib/libbe/be_impl.h
==============================================================================
--- soc2017/kneitinger/libbe-head/lib/libbe/be_impl.h	Wed Jul 19 09:59:32 2017	(r324820)
+++ soc2017/kneitinger/libbe-head/lib/libbe/be_impl.h	Wed Jul 19 12:00:51 2017	(r324821)
@@ -36,16 +36,12 @@
 
 struct libbe_handle {
 	libzfs_handle_t *lzh;
-	zfs_handle_t *be_root;
-	zfs_handle_t *be_active;
+	char root[BE_MAXPATHLEN];
+	char active[BE_MAXPATHLEN];
 	be_error_t error;
-	nvlist_t *list;
 	bool print_on_err;
 };
 
-int prop_list_builder(libbe_handle_t *);
-void prop_list_free(libbe_handle_t *);
-
 int set_error(libbe_handle_t *, be_error_t);
 
 #endif  /* _LIBBE_IMPL_H */

Modified: soc2017/kneitinger/libbe-head/lib/libbe/be_info.c
==============================================================================
--- soc2017/kneitinger/libbe-head/lib/libbe/be_info.c	Wed Jul 19 09:59:32 2017	(r324820)
+++ soc2017/kneitinger/libbe-head/lib/libbe/be_info.c	Wed Jul 19 12:00:51 2017	(r324821)
@@ -29,7 +29,13 @@
 #include "be.h"
 #include "be_impl.h"
 
+typedef struct prop_data {
+	nvlist_t *list;
+	libbe_handle_t *lbh;
+} prop_data_t;
+
 static int prop_list_builder_cb(zfs_handle_t *, void *);
+static int prop_list_builder(prop_data_t *);
 
 /*
  * Returns the name of the active boot environment
@@ -37,9 +43,7 @@
 const char *
 be_active_name(libbe_handle_t *lbh)
 {
-	const char *full_path = zfs_get_name(lbh->be_active);
-
-	return (strrchr(full_path, '/') + 1);
+	return (strrchr(lbh->active, '/') + sizeof(char));
 }
 
 
@@ -49,7 +53,7 @@
 const char *
 be_active_path(libbe_handle_t *lbh)
 {
-	return (zfs_get_name(lbh->be_active));
+	return (lbh->active);
 }
 
 
@@ -59,7 +63,7 @@
 const char *
 be_root_path(libbe_handle_t *lbh)
 {
-	return (zfs_get_name(lbh->be_root));
+	return (lbh->root);
 }
 
 
@@ -70,9 +74,12 @@
 nvlist_t *
 be_get_bootenv_props(libbe_handle_t *lbh)
 {
-	// TODO: Should there be a dirty flag that re-calcs the list if an op
-	// has changed it?
-	return (lbh->list);
+	prop_data_t data;
+
+	data.lbh = lbh;
+	prop_list_builder(&data);
+
+	return (data.list);
 }
 
 
@@ -82,7 +89,7 @@
  * TODO: should any other properties be included?
  */
 static int
-prop_list_builder_cb(zfs_handle_t *zfs_hdl, void *data)
+prop_list_builder_cb(zfs_handle_t *zfs_hdl, void *data_p)
 {
 	/*
 	 * TODO:
@@ -90,13 +97,17 @@
 	 *      error checking
 	 */
 
+	char buf[512];
+	prop_data_t *data;
 	boolean_t mounted, active, nextboot;
 
-	char buf[512];
+
+	data = (prop_data_t *)data_p;
+
 
 	nvlist_t *props;
 
-	libbe_handle_t *lbh = (libbe_handle_t *)data;
+	libbe_handle_t *lbh = data->lbh;
 
 
 	nvlist_alloc(&props, NV_UNIQUE_NAME, KM_SLEEP);
@@ -158,7 +169,7 @@
 
 	/* TODO figure out how to read nextboot (set in libzfs_pool.c) */
 
-	nvlist_add_nvlist(lbh->list, name, props);
+	nvlist_add_nvlist(data->list, name, props);
 
 	return (0);
 }
@@ -170,21 +181,27 @@
  * TODO: ensure that this is always consistent (run after adds, deletes,
  *       renames,etc
  */
-int
-prop_list_builder(libbe_handle_t *lbh)
+static int
+prop_list_builder(prop_data_t *data)
 {
-	if (lbh->list != NULL) {
-		/* TODO: should actually call prop_list_free */
-		nvlist_free(lbh->list);
-		return (1);
-	}
+	zfs_handle_t *root_hdl;
 
-	if (nvlist_alloc(&lbh->list, NV_UNIQUE_NAME, KM_SLEEP) != 0) {
+	if (nvlist_alloc(&(data->list), NV_UNIQUE_NAME, KM_SLEEP) != 0) {
 		/* TODO: actually handle error */
 		return (1);
 	}
 
-	zfs_iter_filesystems(lbh->be_root, prop_list_builder_cb, lbh);
+
+	if ((root_hdl =
+	    zfs_open(data->lbh->lzh, data->lbh->root,
+	    ZFS_TYPE_FILESYSTEM)) == NULL) {
+		return (BE_ERR_ZFSOPEN);
+	}
+
+	// TODO: some error checking here
+	zfs_iter_filesystems(root_hdl, prop_list_builder_cb, data);
+
+	zfs_close(root_hdl);
 
 	return (0);
 }
@@ -194,15 +211,10 @@
  * frees property list and its children
  */
 void
-prop_list_free(libbe_handle_t *lbh)
+be_prop_list_free(nvlist_t *be_list)
 {
-	nvlist_t *be_list;
 	nvlist_t *prop_list;
 
-	if ((be_list = lbh->list) == 0) {
-		return;
-	}
-
 	nvpair_t *be_pair = nvlist_next_nvpair(be_list, NULL);
 
 	if (nvpair_value_nvlist(be_pair, &prop_list) == 0) {


More information about the svn-soc-all mailing list