Skip to contentSkip to conversation

non-const dm_config_find_node

PATCH New Petr Rockai 2 participants 2 messages Aug 31, 2011

lore
Share & tools
lore

Petr Rockai

author
#0 lore raw

PATCH 1/1 non-const dm_config_find_node

Hi,

this patch replaces 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)

Yours,
Petr

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dm_config_find_node.diff
Type: text/x-diff
Size: 19939 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20110831/54fa69e5/attachment.bin>
-------------- next part --------------

Signature · 1 line
id' Ash = Ash; id' Dust = Dust; id' _ = undefined
#1 lore raw

PATCH 1/1 non-const dm_config_find_node

Dne 31.8.2011 16:30, Petr Rockai napsal(a):

Quoted 27 lines

Hi,

this patch replaces 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)

Yours,
Petr

dm_config_find_node.diff

Index: lib/mirror/mirrored.c

Review

Quoted 10 lines
(quoted diff) +1 -6
-	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.",

When we are doing this change maybe there is some way to combine dm_config_get_str() and find_lv() to one internal lvm function call - though error reporting might be tricky so not sure whether it's worth to make some change here, but this call sequence is used several times.

Quoted 19 lines
(quoted diff) +17 -0
@@ -130,14 +125,14 @@ static int _mirrored_text_import(struct 
 
+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)

Review static int _mirrored_text_import(struct

Quoted 10 lines
(quoted diff) +10 -0
+{
+	const struct dm_config_node *n;
+
+	n = _find_config_node(cn, path);
+	if (!n || n->v)
+		return 0;
+
+	*result = n;
+	return 1;
+}

Cosmetic - preferring empty break before return;

ACK, Like this one

Zdenek