From 97a4b5165e172e91a38e48790c769e29360fe768 Mon Sep 17 00:00:00 2001 From: Petr Rockai Date: Wed, 31 Aug 2011 15:19:19 +0000 Subject: [PATCH] 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) --- lib/config/config.c | 6 +-- lib/format_text/import_vsn1.c | 98 +++++++++++++---------------------- lib/format_text/text_import.h | 2 +- lib/mirror/mirrored.c | 17 +++--- lib/raid/raid.c | 15 +++--- lib/striped/striped.c | 6 +-- libdm/libdevmapper.h | 7 ++- libdm/libdm-config.c | 43 +++++++++++++-- 8 files changed, 101 insertions(+), 93 deletions(-) diff --git a/lib/config/config.c b/lib/config/config.c index 39dded0c7..5a7190c3f 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 ecd4b8b3a..b6580785e 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 4f5cde26a..faebc07a1 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 96fa92776..4e16a5cfd 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 f4ba9803b..8bc334400 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 fee2a9628..14d05baab 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 6529b467c..71b4caa11 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 d6a3fe69f..d9e41df5a 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. */