From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10121 invoked by alias); 31 Aug 2011 15:19:21 -0000 Received: (qmail 10103 invoked by uid 9699); 31 Aug 2011 15:19:21 -0000 Date: Wed, 31 Aug 2011 15:19:00 -0000 Message-ID: <20110831151921.10101.qmail@sourceware.org> From: mornfall@sourceware.org To: lvm-devel@redhat.com, lvm2-cvs@sourceware.org Subject: LVM2 lib/config/config.c lib/format_text/impor ... Mailing-List: contact lvm2-cvs-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: lvm2-cvs-owner@sourceware.org X-SW-Source: 2011-08/txt/msg00092.txt.bz2 CVSROOT: /cvs/lvm2 Module name: LVM2 Changes by: mornfall@sourceware.org 2011-08-31 15:19:20 Modified files: lib/config : config.c lib/format_text: import_vsn1.c text_import.h lib/mirror : mirrored.c lib/raid : raid.c lib/striped : striped.c libdm : libdevmapper.h libdm-config.c Log message: 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) Patches: http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/config/config.c.diff?cvsroot=lvm2&r1=1.102&r2=1.103 http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/format_text/import_vsn1.c.diff?cvsroot=lvm2&r1=1.91&r2=1.92 http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/format_text/text_import.h.diff?cvsroot=lvm2&r1=1.6&r2=1.7 http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/mirror/mirrored.c.diff?cvsroot=lvm2&r1=1.90&r2=1.91 http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/raid/raid.c.diff?cvsroot=lvm2&r1=1.10&r2=1.11 http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/striped/striped.c.diff?cvsroot=lvm2&r1=1.38&r2=1.39 http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/libdm/libdevmapper.h.diff?cvsroot=lvm2&r1=1.146&r2=1.147 http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/libdm/libdm-config.c.diff?cvsroot=lvm2&r1=1.2&r2=1.3 --- LVM2/lib/config/config.c 2011/08/30 14:55:16 1.102 +++ LVM2/lib/config/config.c 2011/08/31 15:19:19 1.103 @@ -222,7 +222,7 @@ /* 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 @@ 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 @@ 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) { --- LVM2/lib/format_text/import_vsn1.c 2011/08/30 14:55:17 1.91 +++ LVM2/lib/format_text/import_vsn1.c 2011/08/31 15:19:19 1.92 @@ -108,20 +108,14 @@ 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_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 @@ { 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 @@ 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 @@ { 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 @@ 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 @@ 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 @@ } 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 @@ 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 @@ 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 @@ } 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 @@ } /* 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 @@ /* 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 @@ 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 @@ 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 @@ 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 @@ } /* 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; } --- LVM2/lib/format_text/text_import.h 2011/08/30 14:55:17 1.6 +++ LVM2/lib/format_text/text_import.h 2011/08/31 15:19:19 1.7 @@ -20,7 +20,7 @@ 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 --- LVM2/lib/mirror/mirrored.c 2011/08/30 14:55:17 1.90 +++ LVM2/lib/mirror/mirrored.c 2011/08/31 15:19:20 1.91 @@ -83,10 +83,10 @@ 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 @@ } } - 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 @@ } } - 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 @@ 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) --- LVM2/lib/raid/raid.c 2011/08/30 14:55:18 1.10 +++ LVM2/lib/raid/raid.c 2011/08/31 15:19:20 1.11 @@ -45,10 +45,9 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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; } --- LVM2/lib/striped/striped.c 2011/08/30 14:55:18 1.38 +++ LVM2/lib/striped/striped.c 2011/08/31 15:19:20 1.39 @@ -71,7 +71,7 @@ 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 @@ 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 @@ 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) --- LVM2/libdm/libdevmapper.h 2011/08/31 12:39:58 1.146 +++ LVM2/libdm/libdevmapper.h 2011/08/31 15:19:20 1.147 @@ -1275,7 +1275,8 @@ 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_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); --- LVM2/libdm/libdm-config.c 2011/08/31 12:39:58 1.2 +++ LVM2/libdm/libdm-config.c 2011/08/31 15:19:20 1.3 @@ -973,7 +973,7 @@ * 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_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 @@ { 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 @@ { 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 @@ 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. */