Replace const usage of dm_config_find_node with more appropriate value-lookup

functionality. A number of bugs (copied and pasted all over the code) should
disappear:

- most string lookup based on dm_config_find_node would segfault when
  encountering a non-zero integer (the intention there was to print an
  error message instead)
- check for required sections in metadata would have been satisfied by
  values as well (i.e. not sections)
- encountering a section in place of expected flag value would have
  segfaulted (due to assumed but unchecked cn->v != NULL)

8 files changed, 101 insertions, 93 deletions

diff --git a/lib/config/config.c b/lib/config/config.c
index 39dded0c..5a7190c3 100644
--- a/lib/config/config.c
+++ b/lib/config/config.c
@@ -222,7 +222,7 @@ static void _merge_section(struct dm_config_node *cn1, struct dm_config_node *cn
 			/* Ignore - we don't have any of these yet */
 			continue;
 		/* Not already present? */
-		if (!(oldn = (struct dm_config_node*)dm_config_find_node(cn1->child, cn->key))) {
+		if (!(oldn = dm_config_find_node(cn1->child, cn->key))) {
 			_insert_config_node(&cn1->child, cn);
 			continue;
 		}
@@ -266,7 +266,7 @@ static int _match_host_tags(struct dm_list *tags, const struct dm_config_node *t
 int merge_config_tree(struct cmd_context *cmd, struct dm_config_tree *cft,
 		      struct dm_config_tree *newdata)
 {
-	const struct dm_config_node *root = cft->root;
+	struct dm_config_node *root = cft->root;
 	struct dm_config_node *cn, *nextn, *oldn, *cn2;
 	const struct dm_config_node *tn;
 
@@ -280,7 +280,7 @@ int merge_config_tree(struct cmd_context *cmd, struct dm_config_tree *cft,
 			if (!_match_host_tags(&cmd->tags, tn))
 				continue;
 		}
-		if (!(oldn = (struct dm_config_node *)dm_config_find_node(root, cn->key))) {
+		if (!(oldn = dm_config_find_node(root, cn->key))) {
 			_insert_config_node(&cft->root, cn);
 			/* Remove any "tags" nodes */
 			for (cn2 = cn->child; cn2; cn2 = cn2->sib) {
diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index ecd4b8b3..b6580785 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -108,20 +108,14 @@ static int _is_converting(struct logical_volume *lv)
 
 static int _read_id(struct id *id, const struct dm_config_node *cn, const char *path)
 {
-	const struct dm_config_value *cv;
+	const char *uuid;
 
-	if (!(cn = dm_config_find_node(cn, path))) {
+	if (!dm_config_get_str(cn, path, &uuid)) {
 		log_error("Couldn't find uuid.");
 		return 0;
 	}
 
-	cv = cn->v;
-	if (!cv || !cv->v.str) {
-		log_error("uuid must be a string.");
-		return 0;
-	}
-
-	if (!id_read_format(id, cv->v.str)) {
+	if (!id_read_format(id, uuid)) {
 		log_error("Invalid uuid.");
 		return 0;
 	}
@@ -131,21 +125,21 @@ static int _read_id(struct id *id, const struct dm_config_node *cn, const char *
 
 static int _read_flag_config(const struct dm_config_node *n, uint64_t *status, int type)
 {
-	const struct dm_config_node *cn;
+	const struct dm_config_value *cv;
 	*status = 0;
 
-	if (!(cn = dm_config_find_node(n, "status"))) {
+	if (!dm_config_get_list(n, "status", &cv)) {
 		log_error("Could not find status flags.");
 		return 0;
 	}
 
-	if (!(read_flags(status, type | STATUS_FLAG, cn->v))) {
+	if (!(read_flags(status, type | STATUS_FLAG, cv))) {
 		log_error("Could not read status flags.");
 		return 0;
 	}
 
-	if ((cn = dm_config_find_node(n, "flags"))) {
-		if (!(read_flags(status, type, cn->v))) {
+	if (dm_config_get_list(n, "flags", &cv)) {
+		if (!(read_flags(status, type, cv))) {
 			log_error("Could not read flags.");
 			return 0;
 		}
@@ -164,7 +158,7 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,
 {
 	struct physical_volume *pv;
 	struct pv_list *pvl;
-	const struct dm_config_node *cn;
+	const struct dm_config_value *cv;
 	uint64_t size;
 
 	if (!(pvl = dm_pool_zalloc(mem, sizeof(*pvl))) ||
@@ -238,8 +232,8 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,
 	dm_list_init(&pv->segments);
 
 	/* Optional tags */
-	if ((cn = dm_config_find_node(pvn, "tags")) &&
-	    !(read_tags(mem, &pv->tags, cn->v))) {
+	if (dm_config_get_list(pvn, "tags", &cv) &&
+	    !(read_tags(mem, &pv->tags, cv))) {
 		log_error("Couldn't read tags for physical volume %s in %s.",
 			  pv_dev_name(pv), vg->name);
 		return 0;
@@ -297,7 +291,7 @@ static int _read_segment(struct dm_pool *mem, struct volume_group *vg,
 {
 	uint32_t area_count = 0u;
 	struct lv_segment *seg;
-	const struct dm_config_node *cn, *sn_child = sn->child;
+	const struct dm_config_node *sn_child = sn->child;
 	const struct dm_config_value *cv;
 	uint32_t start_extent, extent_count;
 	struct segment_type *segtype;
@@ -322,13 +316,9 @@ static int _read_segment(struct dm_pool *mem, struct volume_group *vg,
 
 	segtype_str = "striped";
 
-	if ((cn = dm_config_find_node(sn_child, "type"))) {
-		cv = cn->v;
-		if (!cv || !cv->v.str) {
-			log_error("Segment type must be a string.");
-			return 0;
-		}
-		segtype_str = cv->v.str;
+	if (!dm_config_get_str(sn_child, "type", &segtype_str)) {
+		log_error("Segment type must be a string.");
+		return 0;
 	}
 
 	if (!(segtype = get_segtype_from_string(vg->cmd, segtype_str)))
@@ -350,8 +340,8 @@ static int _read_segment(struct dm_pool *mem, struct volume_group *vg,
 		return_0;
 
 	/* Optional tags */
-	if ((cn = dm_config_find_node(sn_child, "tags")) &&
-	    !(read_tags(mem, &seg->tags, cn->v))) {
+	if (dm_config_get_list(sn_child, "tags", &cv) &&
+	    !(read_tags(mem, &seg->tags, cv))) {
 		log_error("Couldn't read tags for a segment of %s/%s.",
 			  vg->name, lv->name);
 		return 0;
@@ -378,11 +368,10 @@ static int _read_segment(struct dm_pool *mem, struct volume_group *vg,
 }
 
 int text_import_areas(struct lv_segment *seg, const struct dm_config_node *sn,
-		      const struct dm_config_node *cn, struct dm_hash_table *pv_hash,
+		      const struct dm_config_value *cv, struct dm_hash_table *pv_hash,
 		      uint64_t status)
 {
 	unsigned int s;
-	const struct dm_config_value *cv;
 	struct logical_volume *lv1;
 	struct physical_volume *pv;
 	const char *seg_name = dm_config_parent_name(sn);
@@ -392,7 +381,7 @@ int text_import_areas(struct lv_segment *seg, const struct dm_config_node *sn,
 		return 0;
 	}
 
-	for (cv = cn->v, s = 0; cv && s < seg->area_count; s++, cv = cv->next) {
+	for (s = 0; cv && s < seg->area_count; s++, cv = cv->next) {
 
 		/* first we read the pv */
 		if (cv->type != DM_CFG_STRING) {
@@ -503,7 +492,8 @@ static int _read_lvnames(struct format_instance *fid __attribute__((unused)),
 			 unsigned report_missing_devices __attribute__((unused)))
 {
 	struct logical_volume *lv;
-	const struct dm_config_node *cn;
+	const char *lv_alloc;
+	const struct dm_config_value *cv;
 
 	if (!(lv = alloc_lv(mem)))
 		return_0;
@@ -523,16 +513,10 @@ static int _read_lvnames(struct format_instance *fid __attribute__((unused)),
 	}
 
 	lv->alloc = ALLOC_INHERIT;
-	if ((cn = dm_config_find_node(lvn, "allocation_policy"))) {
-		const struct dm_config_value *cv = cn->v;
-		if (!cv || !cv->v.str) {
-			log_error("allocation_policy must be a string.");
-			return 0;
-		}
-
-		lv->alloc = get_alloc_from_string(cv->v.str);
+	if (dm_config_get_str(lvn, "allocation_policy", &lv_alloc)) {
+		lv->alloc = get_alloc_from_string(lv_alloc);
 		if (lv->alloc == ALLOC_INVALID) {
-			log_warn("WARNING: Ignoring unrecognised allocation policy %s for LV %s", cv->v.str, lv->name);
+			log_warn("WARNING: Ignoring unrecognised allocation policy %s for LV %s", lv_alloc, lv->name);
 			lv->alloc = ALLOC_INHERIT;
 		}
 	}
@@ -554,8 +538,8 @@ static int _read_lvnames(struct format_instance *fid __attribute__((unused)),
 	}
 
 	/* Optional tags */
-	if ((cn = dm_config_find_node(lvn, "tags")) &&
-	    !(read_tags(mem, &lv->tags, cn->v))) {
+	if (dm_config_get_list(lvn, "tags", &cv) &&
+	    !(read_tags(mem, &lv->tags, cv))) {
 		log_error("Couldn't read tags for logical volume %s/%s.",
 			  vg->name, lv->name);
 		return 0;
@@ -633,7 +617,7 @@ static int _read_sections(struct format_instance *fid,
 	/* Only report missing devices when doing a scan */
 	unsigned report_missing_devices = scan_done_once ? !*scan_done_once : 1;
 
-	if (!(n = dm_config_find_node(vgn, section))) {
+	if (!dm_config_get_section(vgn, section, &n)) {
 		if (!optional) {
 			log_error("Couldn't find section '%s'.", section);
 			return 0;
@@ -655,7 +639,9 @@ static struct volume_group *_read_vg(struct format_instance *fid,
 				     const struct dm_config_tree *cft,
 				     unsigned use_cached_pvs)
 {
-	const struct dm_config_node *vgn, *cn;
+	const struct dm_config_node *vgn;
+	const struct dm_config_value *cv;
+	const char *str;
 	struct volume_group *vg;
 	struct dm_hash_table *pv_hash = NULL, *lv_hash = NULL;
 	unsigned scan_done_once = use_cached_pvs;
@@ -677,12 +663,8 @@ static struct volume_group *_read_vg(struct format_instance *fid,
 
 	vgn = vgn->child;
 
-	if ((cn = dm_config_find_node(vgn, "system_id")) && cn->v) {
-		if (!cn->v->v.str) {
-			log_error("system_id must be a string");
-			goto bad;
-		}
-		strncpy(vg->system_id, cn->v->v.str, NAME_LEN);
+	if (dm_config_get_str(vgn, "system_id", &str)) {
+		strncpy(vg->system_id, str, NAME_LEN);
 	}
 
 	if (!_read_id(&vg->id, vgn, "id")) {
@@ -725,16 +707,10 @@ static struct volume_group *_read_vg(struct format_instance *fid,
 		goto bad;
 	}
 
-	if ((cn = dm_config_find_node(vgn, "allocation_policy"))) {
-		const struct dm_config_value *cv = cn->v;
-		if (!cv || !cv->v.str) {
-			log_error("allocation_policy must be a string.");
-			goto bad;
-		}
-
-		vg->alloc = get_alloc_from_string(cv->v.str);
+	if (dm_config_get_str(vgn, "allocation_policy", &str)) {
+		vg->alloc = get_alloc_from_string(str);
 		if (vg->alloc == ALLOC_INVALID) {
-			log_warn("WARNING: Ignoring unrecognised allocation policy %s for VG %s", cv->v.str, vg->name);
+			log_warn("WARNING: Ignoring unrecognised allocation policy %s for VG %s", str, vg->name);
 			vg->alloc = ALLOC_NORMAL;
 		}
 	}
@@ -760,8 +736,8 @@ static struct volume_group *_read_vg(struct format_instance *fid,
 	}
 
 	/* Optional tags */
-	if ((cn = dm_config_find_node(vgn, "tags")) &&
-	    !(read_tags(vg->vgmem, &vg->tags, cn->v))) {
+	if (dm_config_get_list(vgn, "tags", &cv) &&
+	    !(read_tags(vg->vgmem, &vg->tags, cv))) {
 		log_error("Couldn't read tags for volume group %s.", vg->name);
 		goto bad;
 	}
diff --git a/lib/format_text/text_import.h b/lib/format_text/text_import.h
index 4f5cde26..faebc07a 100644
--- a/lib/format_text/text_import.h
+++ b/lib/format_text/text_import.h
@@ -20,7 +20,7 @@ struct lv_segment;
 struct dm_config_node;
 
 int text_import_areas(struct lv_segment *seg, const struct dm_config_node *sn,
-		      const struct dm_config_node *cn, struct dm_hash_table *pv_hash,
+		      const struct dm_config_value *cv, struct dm_hash_table *pv_hash,
 		      uint64_t status);
 
 #endif
diff --git a/lib/mirror/mirrored.c b/lib/mirror/mirrored.c
index 96fa9277..4e16a5cf 100644
--- a/lib/mirror/mirrored.c
+++ b/lib/mirror/mirrored.c
@@ -83,10 +83,10 @@ static int _mirrored_text_import_area_count(const struct dm_config_node *sn, uin
 static int _mirrored_text_import(struct lv_segment *seg, const struct dm_config_node *sn,
 			struct dm_hash_table *pv_hash)
 {
-	const struct dm_config_node *cn;
+	const struct dm_config_value *cv;
 	const char *logname = NULL;
 
-	if (dm_config_find_node(sn, "extents_moved")) {
+	if (dm_config_has_node(sn, "extents_moved")) {
 		if (dm_config_get_uint32(sn, "extents_moved",
 				      &seg->extents_copied))
 			seg->status |= PVMOVE;
@@ -98,7 +98,7 @@ static int _mirrored_text_import(struct lv_segment *seg, const struct dm_config_
 		}
 	}
 
-	if (dm_config_find_node(sn, "region_size")) {
+	if (dm_config_has_node(sn, "region_size")) {
 		if (!dm_config_get_uint32(sn, "region_size",
 				      &seg->region_size)) {
 			log_error("Couldn't read 'region_size' for "
@@ -108,12 +108,7 @@ static int _mirrored_text_import(struct lv_segment *seg, const struct dm_config_
 		}
 	}
 
-	if ((cn = dm_config_find_node(sn, "mirror_log"))) {
-		if (!cn->v || !cn->v->v.str) {
-			log_error("Mirror log type must be a string.");
-			return 0;
-		}
-		logname = cn->v->v.str;
+	if (dm_config_get_str(sn, "mirror_log", &logname)) {
 		if (!(seg->log_lv = find_lv(seg->lv->vg, logname))) {
 			log_error("Unrecognised mirror log in "
 				  "segment %s of logical volume %s.",
@@ -130,14 +125,14 @@ static int _mirrored_text_import(struct lv_segment *seg, const struct dm_config_
 		return 0;
 	}
 
-	if (!(cn = dm_config_find_node(sn, "mirrors"))) {
+	if (!dm_config_get_list(sn, "mirrors", &cv)) {
 		log_error("Couldn't find mirrors array for "
 			  "segment %s of logical volume %s.",
 			  dm_config_parent_name(sn), seg->lv->name);
 		return 0;
 	}
 
-	return text_import_areas(seg, sn, cn, pv_hash, MIRROR_IMAGE);
+	return text_import_areas(seg, sn, cv, pv_hash, MIRROR_IMAGE);
 }
 
 static int _mirrored_text_export(const struct lv_segment *seg, struct formatter *f)
diff --git a/lib/raid/raid.c b/lib/raid/raid.c
index f4ba9803..8bc33440 100644
--- a/lib/raid/raid.c
+++ b/lib/raid/raid.c
@@ -45,10 +45,9 @@ static int _raid_text_import_area_count(const struct dm_config_node *sn,
 
 static int _raid_text_import_areas(struct lv_segment *seg,
 				   const struct dm_config_node *sn,
-				   const struct dm_config_node *cn)
+				   const struct dm_config_value *cv)
 {
 	unsigned int s;
-	const struct dm_config_value *cv;
 	struct logical_volume *lv1;
 	const char *seg_name = dm_config_parent_name(sn);
 
@@ -57,7 +56,7 @@ static int _raid_text_import_areas(struct lv_segment *seg,
 		return 0;
 	}
 
-	for (cv = cn->v, s = 0; cv && s < seg->area_count; s++, cv = cv->next) {
+	for (s = 0; cv && s < seg->area_count; s++, cv = cv->next) {
 		if (cv->type != DM_CFG_STRING) {
 			log_error("Bad volume name in areas array for segment %s.", seg_name);
 			return 0;
@@ -104,9 +103,9 @@ static int _raid_text_import(struct lv_segment *seg,
 			     const struct dm_config_node *sn,
 			     struct dm_hash_table *pv_hash)
 {
-	const struct dm_config_node *cn;
+	const struct dm_config_value *cv;
 
-	if (dm_config_find_node(sn, "region_size")) {
+	if (dm_config_has_node(sn, "region_size")) {
 		if (!dm_config_get_uint32(sn, "region_size", &seg->region_size)) {
 			log_error("Couldn't read 'region_size' for "
 				  "segment %s of logical volume %s.",
@@ -114,7 +113,7 @@ static int _raid_text_import(struct lv_segment *seg,
 			return 0;
 		}
 	}
-	if (dm_config_find_node(sn, "stripe_size")) {
+	if (dm_config_has_node(sn, "stripe_size")) {
 		if (!dm_config_get_uint32(sn, "stripe_size", &seg->stripe_size)) {
 			log_error("Couldn't read 'stripe_size' for "
 				  "segment %s of logical volume %s.",
@@ -122,14 +121,14 @@ static int _raid_text_import(struct lv_segment *seg,
 			return 0;
 		}
 	}
-	if (!(cn = dm_config_find_node(sn, "raids"))) {
+	if (!dm_config_get_list(sn, "raids", &cv)) {
 		log_error("Couldn't find RAID array for "
 			  "segment %s of logical volume %s.",
 			  dm_config_parent_name(sn), seg->lv->name);
 		return 0;
 	}
 
-	if (!_raid_text_import_areas(seg, sn, cn)) {
+	if (!_raid_text_import_areas(seg, sn, cv)) {
 		log_error("Failed to import RAID images");
 		return 0;
 	}
diff --git a/lib/striped/striped.c b/lib/striped/striped.c
index fee2a962..14d05baa 100644
--- a/lib/striped/striped.c
+++ b/lib/striped/striped.c
@@ -71,7 +71,7 @@ static int _striped_text_import_area_count(const struct dm_config_node *sn, uint
 static int _striped_text_import(struct lv_segment *seg, const struct dm_config_node *sn,
 			struct dm_hash_table *pv_hash)
 {
-	const struct dm_config_node *cn;
+	const struct dm_config_value *cv;
 
 	if ((seg->area_count != 1) &&
 	    !dm_config_get_uint32(sn, "stripe_size", &seg->stripe_size)) {
@@ -80,7 +80,7 @@ static int _striped_text_import(struct lv_segment *seg, const struct dm_config_n
 		return 0;
 	}
 
-	if (!(cn = dm_config_find_node(sn, "stripes"))) {
+	if (!dm_config_get_list(sn, "stripes", &cv)) {
 		log_error("Couldn't find stripes array for segment %s "
 			  "of logical volume %s.", dm_config_parent_name(sn), seg->lv->name);
 		return 0;
@@ -88,7 +88,7 @@ static int _striped_text_import(struct lv_segment *seg, const struct dm_config_n
 
 	seg->area_len /= seg->area_count;
 
-	return text_import_areas(seg, sn, cn, pv_hash, 0);
+	return text_import_areas(seg, sn, cv, pv_hash, 0);
 }
 
 static int _striped_text_export(const struct lv_segment *seg, struct formatter *f)
diff --git a/libdm/libdevmapper.h b/libdm/libdevmapper.h
index 6529b467..71b4caa1 100644
--- a/libdm/libdevmapper.h
+++ b/libdm/libdevmapper.h
@@ -1275,7 +1275,8 @@ int dm_config_write_node(const struct dm_config_node *cn, dm_putline_fn putline,
 time_t dm_config_timestamp(struct dm_config_tree *cft);
 int dm_config_changed(struct dm_config_tree *cft);
 
-struct dm_config_node *dm_config_find_node(const struct dm_config_node *cn, const char *path);
+struct dm_config_node *dm_config_find_node(struct dm_config_node *cn, const char *path);
+int dm_config_has_node(const struct dm_config_node *cn, const char *path);
 const char *dm_config_find_str(const struct dm_config_node *cn, const char *path, const char *fail);
 int dm_config_find_int(const struct dm_config_node *cn, const char *path, int fail);
 float dm_config_find_float(const struct dm_config_node *cn, const char *path, float fail);
@@ -1307,6 +1308,10 @@ int dm_config_get_uint64(const struct dm_config_node *cn, const char *path,
 
 int dm_config_get_str(const struct dm_config_node *cn, const char *path,
 		      const char **result);
+int dm_config_get_list(const struct dm_config_node *cn, const char *path,
+		       const struct dm_config_value **result);
+int dm_config_get_section(const struct dm_config_node *cn, const char *path,
+			  const struct dm_config_node **result);
 
 unsigned dm_config_maybe_section(const char *str, unsigned len);
 
diff --git a/libdm/libdm-config.c b/libdm/libdm-config.c
index d6a3fe69..d9e41df5 100644
--- a/libdm/libdm-config.c
+++ b/libdm/libdm-config.c
@@ -973,7 +973,7 @@ static int _find_config_bool(const void *start, _node_lookup_fn find,
  * node-based lookup
  **/
 
-struct dm_config_node *dm_config_find_node(const struct dm_config_node *cn,
+struct dm_config_node *dm_config_find_node(struct dm_config_node *cn,
 					   const char *path)
 {
 	return _find_config_node(cn, path);
@@ -1038,11 +1038,11 @@ int dm_config_tree_find_bool(const struct dm_config_tree *cft, const char *path,
 
 
 int dm_config_get_uint32(const struct dm_config_node *cn, const char *path,
-			      uint32_t *result)
+			 uint32_t *result)
 {
 	const struct dm_config_node *n;
 
-	n = dm_config_find_node(cn, path);
+	n = _find_config_node(cn, path);
 
 	if (!n || !n->v || n->v->type != DM_CFG_INT)
 		return 0;
@@ -1056,7 +1056,7 @@ int dm_config_get_uint64(const struct dm_config_node *cn, const char *path,
 {
 	const struct dm_config_node *n;
 
-	n = dm_config_find_node(cn, path);
+	n = _find_config_node(cn, path);
 
 	if (!n || !n->v || n->v->type != DM_CFG_INT)
 		return 0;
@@ -1070,7 +1070,7 @@ int dm_config_get_str(const struct dm_config_node *cn, const char *path,
 {
 	const struct dm_config_node *n;
 
-	n = dm_config_find_node(cn, path);
+	n = _find_config_node(cn, path);
 
 	if (!n || !n->v || n->v->type != DM_CFG_STRING)
 		return 0;
@@ -1079,6 +1079,39 @@ int dm_config_get_str(const struct dm_config_node *cn, const char *path,
 	return 1;
 }
 
+int dm_config_get_list(const struct dm_config_node *cn, const char *path,
+		       const struct dm_config_value **result)
+{
+	const struct dm_config_node *n;
+
+	n = _find_config_node(cn, path);
+	/* TODO when we represent single-item lists consistently, add a check
+	 * for n->v->next != NULL */
+	if (!n || !n->v)
+		return 0;
+
+	*result = n->v;
+	return 1;
+}
+
+int dm_config_get_section(const struct dm_config_node *cn, const char *path,
+			  const struct dm_config_node **result)
+{
+	const struct dm_config_node *n;
+
+	n = _find_config_node(cn, path);
+	if (!n || n->v)
+		return 0;
+
+	*result = n;
+	return 1;
+}
+
+int dm_config_has_node(const struct dm_config_node *cn, const char *path)
+{
+	return _find_config_node(cn, path) ? 1 : 0;
+}
+
 /*
  * Convert a token type to the char it represents.
  */