[lvm-devel] [PATCH] Fix confusing metadta syntax error messages.
Dave Wysochanski
dwysocha at redhat.com
Fri Jul 3 13:07:49 UTC 2009
On Wed, 2009-07-01 at 18:08 +0200, Milan Broz wrote:
> Fix confusing metadta syntax error messages.
>
> If there is syntax error in metadata, it now prints messages
> like:
> Couldn't read 'start_extent' for segment 'extent_count'.
> Couldn't read all logical volumes for volume group vg_test.
>
> The segment specification is wrong and confusing.
>
> To provide better information to user, we should print at least
> proper section name or, if possible, even LV section where is
> the syntax problem.
>
> Patch fixes it by introducing "parent" member in config_node which
> points to parent section and config_node_name function, which
> provides pointer to node section name.
>
> Also it adds several LV refernces where possible.
>
Nice cleanup. I tested this and much better messages. Minor comments
below and small fixup patch attached.
> Signed-off-by: Milan Broz <mbroz at redhat.com>
> ---
> lib/config/config.c | 9 +++++++++
> lib/config/config.h | 4 +++-
> lib/format_text/import_vsn1.c | 34 +++++++++++-----------------------
> lib/mirror/mirrored.c | 23 ++++++++++++++---------
> lib/striped/striped.c | 10 +++++-----
> 5 files changed, 42 insertions(+), 38 deletions(-)
>
> diff --git a/lib/config/config.c b/lib/config/config.c
> index 224f2ce..a14beac 100644
> --- a/lib/config/config.c
> +++ b/lib/config/config.c
> @@ -546,6 +546,7 @@ static struct config_node *_file(struct parser *p)
> root = n;
> else
> l->sib = n;
> + n->parent = root;
> l = n;
> }
> return root;
I was a little unsure about this - should we use "NULL" for a root node
instead of n->parent == n? Your approach looks ok given how the code is
currently written.
> @@ -573,6 +574,7 @@ static struct config_node *_section(struct parser *p)
> root->child = n;
> else
> l->sib = n;
> + n->parent = root;
> l = n;
> }
> match(TOK_SECTION_E);
> @@ -1251,6 +1253,13 @@ static unsigned _count_tokens(const char *str, unsigned len, int type)
> return count_chars(str, len, c);
> }
>
> +const char *config_node_name(const struct config_node *n)
> +{
> + if (n->parent)
> + return n->parent->key;
> +
> + return n->key;
> +}
This is a little unclear. Instead, how about:
const char *config_section_name(const struct config_node *n)
{
return (n->parent ? n->parent->key : "root");
}
const char *config_node_name(const struct config_node *n)
{
return n->key;
}
Then use config_section_name() in most places below - it is the section
name of the node you display below.
Problem is the code does not really distinguish between a node that is a
config section and one that is not. I thought of using parent but
perhaps config_node_parent_name() is a bit long.
> /*
> * Heuristic function to make a quick guess as to whether a text
> * region probably contains a valid config "section". (Useful for
> diff --git a/lib/config/config.h b/lib/config/config.h
> index 57f6c32..7da4600 100644
> --- a/lib/config/config.h
> +++ b/lib/config/config.h
> @@ -40,7 +40,7 @@ struct config_value {
>
> struct config_node {
> char *key;
> - struct config_node *sib, *child;
> + struct config_node *parent, *sib, *child;
> struct config_value *v;
> };
>
> @@ -110,4 +110,6 @@ int get_config_str(const struct config_node *cn, const char *path,
>
> unsigned maybe_config_section(const char *str, unsigned len);
>
> +const char *config_node_name(const struct config_node *n);
> +
> #endif
> diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
> index d8d8fcb..35253e5 100644
> --- a/lib/format_text/import_vsn1.c
> +++ b/lib/format_text/import_vsn1.c
> @@ -305,14 +305,14 @@ static int _read_segment(struct dm_pool *mem, struct volume_group *vg,
> }
>
> if (!_read_int32(sn, "start_extent", &start_extent)) {
> - log_error("Couldn't read 'start_extent' for segment '%s'.",
> - sn->key);
> + log_error("Couldn't read 'start_extent' for segment '%s' "
> + "of logical volume %s.", config_node_name(sn), lv->name);
> return 0;
> }
>
> if (!_read_int32(sn, "extent_count", &extent_count)) {
> - log_error("Couldn't read 'extent_count' for segment '%s'.",
> - sn->key);
> + log_error("Couldn't read 'extent_count' for segment '%s' "
> + "of logical volume %s.", config_node_name(sn), lv->name);
> return 0;
> }
>
> @@ -377,32 +377,19 @@ int text_import_areas(struct lv_segment *seg, const struct config_node *sn,
> unsigned int s;
> struct config_value *cv;
> struct logical_volume *lv1;
> - const char *seg_name = sn->key;
> + struct physical_volume *pv;
> + const char *seg_name = config_node_name(sn);
>
> if (!seg->area_count) {
> - log_error("Zero areas not allowed for segment '%s'", sn->key);
> + log_error("Zero areas not allowed for segment '%s'", seg_name);
> return 0;
> }
>
> for (cv = cn->v, s = 0; cv && s < seg->area_count; s++, cv = cv->next) {
>
> /* first we read the pv */
> - const char *bad = "Badly formed areas array for "
> - "segment '%s'.";
> - struct physical_volume *pv;
> -
> - if (cv->type != CFG_STRING) {
> - log_error(bad, sn->key);
> - return 0;
> - }
> -
> - if (!cv->next) {
> - log_error(bad, sn->key);
> - return 0;
> - }
> -
> - if (cv->next->type != CFG_INT) {
> - log_error(bad, sn->key);
> + if (cv->type != CFG_STRING || !cv->next || cv->next->type != CFG_INT) {
> + log_error("Badly formed areas array for segment '%s'.", seg_name);
> return 0;
> }
>
> @@ -463,7 +450,8 @@ static int _read_segments(struct dm_pool *mem, struct volume_group *vg,
> }
>
> if (!_read_int32(lvn, "segment_count", &seg_count)) {
> - log_error("Couldn't read segment count for logical volume.");
> + log_error("Couldn't read segment count for logical volume %s.",
> + lv->name);
> return 0;
> }
>
> diff --git a/lib/mirror/mirrored.c b/lib/mirror/mirrored.c
> index b243ebb..4d0fc63 100644
> --- a/lib/mirror/mirrored.c
> +++ b/lib/mirror/mirrored.c
> @@ -78,7 +78,7 @@ static int _mirrored_text_import_area_count(struct config_node *sn, uint32_t *ar
> {
> if (!get_config_uint32(sn, "mirror_count", area_count)) {
> log_error("Couldn't read 'mirror_count' for "
> - "segment '%s'.", sn->key);
> + "segment '%s'.", config_node_name(sn));
> return 0;
> }
>
Note that you could enhance this easily to also print the LV name by
using config*name(sn->parent).
> @@ -97,7 +97,8 @@ static int _mirrored_text_import(struct lv_segment *seg, const struct config_nod
> seg->status |= PVMOVE;
> else {
> log_error("Couldn't read 'extents_moved' for "
> - "segment '%s'.", sn->key);
> + "segment %s of logical volume %s.",
> + config_node_name(sn), seg->lv->name);
> return 0;
> }
> }
> @@ -106,7 +107,8 @@ static int _mirrored_text_import(struct lv_segment *seg, const struct config_nod
> if (!get_config_uint32(sn, "region_size",
> &seg->region_size)) {
> log_error("Couldn't read 'region_size' for "
> - "segment '%s'.", sn->key);
> + "segment %s of logical volume %s.",
> + config_node_name(sn), seg->lv->name);
> return 0;
> }
> }
> @@ -118,22 +120,25 @@ static int _mirrored_text_import(struct lv_segment *seg, const struct config_nod
> }
> logname = cn->v->v.str;
> if (!(seg->log_lv = find_lv(seg->lv->vg, logname))) {
> - log_error("Unrecognised mirror log in segment %s.",
> - sn->key);
> + log_error("Unrecognised mirror log in "
> + "segment %s of logical volume %s.",
> + config_node_name(sn), seg->lv->name);
> return 0;
> }
> seg->log_lv->status |= MIRROR_LOG;
> }
>
> if (logname && !seg->region_size) {
> - log_error("Missing region size for mirror log for segment "
> - "'%s'.", sn->key);
> + log_error("Missing region size for mirror log for "
> + "segment %s of logical volume %s.",
> + config_node_name(sn), seg->lv->name);
> return 0;
> }
>
> if (!(cn = find_config_node(sn, "mirrors"))) {
> - log_error("Couldn't find mirrors array for segment "
> - "'%s'.", sn->key);
> + log_error("Couldn't find mirrors array for "
> + "segment %s of logical volume %s.",
> + config_node_name(sn), seg->lv->name);
> return 0;
> }
>
> diff --git a/lib/striped/striped.c b/lib/striped/striped.c
> index 78129af..6dda69b 100644
> --- a/lib/striped/striped.c
> +++ b/lib/striped/striped.c
> @@ -54,7 +54,7 @@ static int _striped_text_import_area_count(struct config_node *sn, uint32_t *are
> {
> if (!get_config_uint32(sn, "stripe_count", area_count)) {
> log_error("Couldn't read 'stripe_count' for "
> - "segment '%s'.", sn->key);
> + "segment '%s'.", config_node_name(sn));
> return 0;
> }
Same as above.
>
> @@ -68,14 +68,14 @@ static int _striped_text_import(struct lv_segment *seg, const struct config_node
>
> if ((seg->area_count != 1) &&
> !get_config_uint32(sn, "stripe_size", &seg->stripe_size)) {
> - log_error("Couldn't read stripe_size for segment '%s'.",
> - sn->key);
> + log_error("Couldn't read stripe_size for segment %s "
> + "of logical volume %s.", config_node_name(sn), seg->lv->name);
> return 0;
> }
>
> if (!(cn = find_config_node(sn, "stripes"))) {
> - log_error("Couldn't find stripes array for segment "
> - "'%s'.", sn->key);
> + log_error("Couldn't find stripes array for segment %s "
> + "of logical volume %s.", config_node_name(sn), seg->lv->name);
> return 0;
> }
>
>
Ack.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Minor-fixups-of-milan-s-config-cleanup.patch
Type: text/x-patch
Size: 6245 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090703/bac77300/attachment.bin>
More information about the lvm-devel
mailing list