Skip to contentSkip to conversation

ceph: improve error handling in ceph_mdsmap_decode

PATCH Applied Emil Goode 5 participants 6 messages May 28, 2013 – May 29, 2013

Subsystems (2)
Kernel janitors Ceph client
lore
Share & tools
lore

Emil Goode

author
#0 lore raw

PATCH 1/1 ceph: improve error handling in ceph_mdsmap_decode

This patch makes the following improvements to the error handling in the ceph_mdsmap_decode function:

- Add a NULL check for return value from kcalloc
- Make use of the variable err

Signed-off-by Emil Goode <[email protected]>
---
 fs/ceph/mdsmap.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/ceph/mdsmap.c +3 -1
index 9278dec..d4d3897 100644
@@ -138,6 +138,8 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
 				m->m_info[mds].export_targets  					kcalloc(num_export_targets, sizeof(u32),
 						GFP_NOFS);
+				if (m->m_info[mds].export_targets = NULL)
+					goto badmem;
 				for (j = 0; j < num_export_targets; j++)
 					m->m_info[mds].export_targets[j]  					       ceph_decode_32(&pexport_targets);

@@ -170,7 +172,7 @@ bad:
 		       DUMP_PREFIX_OFFSET, 16, 1,
 		       start, end - start, true);
 	ceph_mdsmap_destroy(m);
-	return ERR_PTR(-EINVAL);
+	return ERR_PTR(err);
 }
 
 void ceph_mdsmap_destroy(struct ceph_mdsmap *m)
Signature · 1 line
1.7.10.4
Applied
#1 lore raw

Applied to ceph tree; thanks!

Quoted 42 lines · Emil Goode

This patch makes the following improvements to the error handling in the ceph_mdsmap_decode function:

- Add a NULL check for return value from kcalloc
- Make use of the variable err

Signed-off-by Emil Goode <[email protected]>
---
 fs/ceph/mdsmap.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/ceph/mdsmap.c +3 -1
index 9278dec..d4d3897 100644
@@ -138,6 +138,8 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
 				m->m_info[mds].export_targets =
 					kcalloc(num_export_targets, sizeof(u32),
 						GFP_NOFS);
+				if (m->m_info[mds].export_targets == NULL)
+					goto badmem;
 				for (j = 0; j < num_export_targets; j++)
 					m->m_info[mds].export_targets[j] =
 					       ceph_decode_32(&pexport_targets);

@@ -170,7 +172,7 @@ bad:
 		       DUMP_PREFIX_OFFSET, 16, 1,
 		       start, end - start, true);
 	ceph_mdsmap_destroy(m);
-	return ERR_PTR(-EINVAL);
+	return ERR_PTR(err);
 }
 
 void ceph_mdsmap_destroy(struct ceph_mdsmap *m)
Signature · 6 lines
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#2 lore raw

PATCH 1/1 ceph: tidy ceph_mdsmap_decode() a little

In reply to Emil Goode

I introduced a new temporary variable "info" instead of "m->m_info[mds]". Also I reversed the if condition and pulled everything in one indent level.

Signed-off-by Dan Carpenter <[email protected]>

---
This goes on top of Emil Goode's patch.

fs/ceph/mdsmap.c +22 -20
index d4d3897..132b64e 100644
@@ -92,6 +92,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
 		u32 num_export_targets;
 		void *pexport_targets = NULL;
 		struct ceph_timespec laggy_since;
+		struct ceph_mds_info *info;
 
 		ceph_decode_need(p, end, sizeof(u64)*2 + 1 + sizeof(u32), bad);
 		global_id = ceph_decode_64(p);

@@ -126,26 +127,27 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
 		     i+1, n, global_id, mds, inc,
 		     ceph_pr_addr(&addr.in_addr),
 		     ceph_mds_state_name(state));
-		if (mds >= 0 && mds < m->m_max_mds && state > 0) {
-			m->m_info[mds].global_id = global_id;
-			m->m_info[mds].state = state;
-			m->m_info[mds].addr = addr;
-			m->m_info[mds].laggy =
-				(laggy_since.tv_sec != 0 ||
-				 laggy_since.tv_nsec != 0);
-			m->m_info[mds].num_export_targets = num_export_targets;
-			if (num_export_targets) {
-				m->m_info[mds].export_targets =
-					kcalloc(num_export_targets, sizeof(u32),
-						GFP_NOFS);
-				if (m->m_info[mds].export_targets == NULL)
-					goto badmem;
-				for (j = 0; j < num_export_targets; j++)
-					m->m_info[mds].export_targets[j] =
-					       ceph_decode_32(&pexport_targets);
-			} else {
-				m->m_info[mds].export_targets = NULL;
-			}
+
+		if (mds < 0 || mds >= m->m_max_mds || state <= 0)
+			continue;
+
+		info = &m->m_info[mds];
+		info->global_id = global_id;
+		info->state = state;
+		info->addr = addr;
+		info->laggy = (laggy_since.tv_sec != 0 ||
+			       laggy_since.tv_nsec != 0);
+		info->num_export_targets = num_export_targets;
+		if (num_export_targets) {
+			info->export_targets = kcalloc(num_export_targets,
+						       sizeof(u32), GFP_NOFS);
+			if (info->export_targets == NULL)
+				goto badmem;
+			for (j = 0; j < num_export_targets; j++)
+				info->export_targets[j] =
+				       ceph_decode_32(&pexport_targets);
+		} else {
+			info->export_targets = NULL;
 		}
 	}
#3 lore raw

Re: [patch] ceph: tidy ceph_mdsmap_decode() a little

Am 29.05.2013 08:22, schrieb Dan Carpenter:

Quoted 45 lines

I introduced a new temporary variable "info" instead of "m->m_info[mds]". Also I reversed the if condition and pulled everything in one indent level.

Signed-off-by Dan Carpenter <[email protected]>

---
This goes on top of Emil Goode's patch.

fs/ceph/mdsmap.c +2 -20
index d4d3897..132b64e 100644
@@ -92,6 +92,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
 		u32 num_export_targets;
 		void *pexport_targets = NULL;
 		struct ceph_timespec laggy_since;
+		struct ceph_mds_info *info;
 
 		ceph_decode_need(p, end, sizeof(u64)*2 + 1 + sizeof(u32), bad);
 		global_id = ceph_decode_64(p);

@@ -126,26 +127,27 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
 		     i+1, n, global_id, mds, inc,
 		     ceph_pr_addr(&addr.in_addr),
 		     ceph_mds_state_name(state));
-		if (mds >= 0 && mds < m->m_max_mds && state > 0) {
-			m->m_info[mds].global_id = global_id;
-			m->m_info[mds].state = state;
-			m->m_info[mds].addr = addr;
-			m->m_info[mds].laggy =
-				(laggy_since.tv_sec != 0 ||
-				 laggy_since.tv_nsec != 0);
-			m->m_info[mds].num_export_targets = num_export_targets;
-			if (num_export_targets) {
-				m->m_info[mds].export_targets =
-					kcalloc(num_export_targets, sizeof(u32),
-						GFP_NOFS);
-				if (m->m_info[mds].export_targets == NULL)
-					goto badmem;
-				for (j = 0; j < num_export_targets; j++)
-					m->m_info[mds].export_targets[j] =
-					       ceph_decode_32(&pexport_targets);
-			} else {
-				m->m_info[mds].export_targets = NULL;
-			}
+

fs/ceph/mdsmap.c struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) · line 140

Quoted 10 lines
fs/ceph/mdsmap.c +10 -0
+		if (mds < 0 || mds >= m->m_max_mds || state <= 0)
+			continue;
+
+		info = &m->m_info[mds];
+		info->global_id = global_id;
+		info->state = state;
+		info->addr = addr;
+		info->laggy = (laggy_since.tv_sec != 0 ||
+			       laggy_since.tv_nsec != 0);
+		info->num_export_targets = num_export_targets;

personally i would go for:

		info->export_targets = NULL;

and remove the else below.

hope that helps,

re,
wh

Quoted 18 lines
fs/ceph/mdsmap.c +10 -1
+		if (num_export_targets) {
+			info->export_targets = kcalloc(num_export_targets,
+						       sizeof(u32), GFP_NOFS);
+			if (info->export_targets == NULL)
+				goto badmem;
+			for (j = 0; j < num_export_targets; j++)
+				info->export_targets[j] =
+				       ceph_decode_32(&pexport_targets);
+		} else {
+			info->export_targets = NULL;
 		}
 	}
 
--

To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw

Re: [patch] ceph: tidy ceph_mdsmap_decode() a little

Quoted 4 lines · walter harms

personally i would go for:

		info->export_targets = NULL;

and remove the else below.

I don't have strong feelings one way or the other, but honestly, I think the way I sent it is more clear.

regards,
dan carpenter

Reviewed-by on v1 1/1
#5 lore raw

Re: [patch] ceph: tidy ceph_mdsmap_decode() a little

In reply to Dan Carpenter
Quoted 3 lines · Dan Carpenter

I introduced a new temporary variable "info" instead of "m->m_info[mds]". Also I reversed the if condition and pulled everything in one indent level.

Looks good. I will apply this for you.

Reviewed-by Alex Elder <[email protected]>
Quoted 10 lines
Signed-off-by Dan Carpenter <[email protected]>

---
This goes on top of Emil Goode's patch.

fs/ceph/mdsmap.c +0 -0
index d4d3897..132b64e 100644

fs/ceph/mdsmap.c struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) · line 98

Quoted 7 lines
fs/ceph/mdsmap.c +1 -0
@@ -92,6 +92,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
 		u32 num_export_targets;
 		void *pexport_targets = NULL;
 		struct ceph_timespec laggy_since;
+		struct ceph_mds_info *info;
 
 		ceph_decode_need(p, end, sizeof(u64)*2 + 1 + sizeof(u32), bad);
 		global_id = ceph_decode_64(p);

. . .