* [PATCH] Fix confusing metadta syntax error messages.
@ 2009-07-01 16:08 Milan Broz
2009-07-03 13:07 ` Dave Wysochanski
0 siblings, 1 reply; 2+ messages in thread
From: Milan Broz @ 2009-07-01 16:08 UTC (permalink / raw)
To: lvm-devel
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.
Signed-off-by: Milan Broz <mbroz@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;
@@ -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;
+}
/*
* 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;
}
@@ -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;
}
@@ -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;
}
^ permalink raw reply related [flat|nested] 2+ messages in thread
* [PATCH] Fix confusing metadta syntax error messages.
2009-07-01 16:08 [PATCH] Fix confusing metadta syntax error messages Milan Broz
@ 2009-07-03 13:07 ` Dave Wysochanski
0 siblings, 0 replies; 2+ messages in thread
From: Dave Wysochanski @ 2009-07-03 13:07 UTC (permalink / raw)
To: lvm-devel
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@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>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2009-07-03 13:07 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-07-01 16:08 [PATCH] Fix confusing metadta syntax error messages Milan Broz
2009-07-03 13:07 ` Dave Wysochanski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.