Skip to contentSkip to conversation

staging: lustre: checking for NULL instead of IS_ERR

PATCH In review Dan Carpenter 2 participants 3 messages Mar 18, 2016

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 staging: lustre: checking for NULL instead of IS_ERR

lustre_cfg_new() returns error pointers on error, it never returns NULL.

drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -1
index 65caffe..b7dc872 100644
@@ -1282,7 +1282,7 @@ static int mgc_apply_recover_logs(struct obd_device *mgc,
 
 		rc = -ENOMEM;
 		lcfg = lustre_cfg_new(LCFG_PARAM, &bufs);
-		if (!lcfg) {
+		if (IS_ERR(lcfg)) {
 			CERROR("mgc: cannot allocate memory\n");
 			break;
 		}
Reviewed-by on v1 1/1
#1 lore raw
Quoted 18 lines · Dan Carpenter

lustre_cfg_new() returns error pointers on error, it never returns NULL.

drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -1
index 65caffe..b7dc872 100644
@@ -1282,7 +1282,7 @@ static int mgc_apply_recover_logs(struct obd_device
 *mgc,
 
 		rc = -ENOMEM;
 		lcfg = lustre_cfg_new(LCFG_PARAM, &bufs);
-		if (!lcfg) {
+		if (IS_ERR(lcfg)) {
 			CERROR("mgc: cannot allocate memory\n");
 			break;
Quoted 1 line
		}

Looks good. Thanks for the patch.

Cheers, Andreas

Signature · 4 lines
Andreas Dilger

Lustre Principal Architect
Intel High Performance Data Division
#2 lore raw
Quoted 3 lines · Andreas
Quoted 1 line · Dan Carpenter

lustre_cfg_new() returns error pointers on error, it never returns NULL.

Hmm, looking at the broader context, I see there are other callers of lustre_cfg_new() that are checking for NULL, and in some cases not checking the return code at all.

I'm not sure why Peng Tao changed the return value of this function to ERR_PTR() instead of NULL, but it may be better to have lustre_cfg_new() return NULL on failure to avoid changing more code, and because it is essentially an alloc+init function and the only error it could ever return is -ENOMEM and typically allocator functions return NULL.

Cheers, Andreas

Quoted 20 lines
Quoted 16 lines
drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -1
index 65caffe..b7dc872 100644
@@ -1282,7 +1282,7 @@ static int mgc_apply_recover_logs(struct obd_device
 *mgc,
 
 		rc = -ENOMEM;
 		lcfg = lustre_cfg_new(LCFG_PARAM, &bufs);
-		if (!lcfg) {
+		if (IS_ERR(lcfg)) {
 			CERROR("mgc: cannot allocate memory\n");
 			break;
Quoted 1 line
		}

Looks good. Thanks for the patch.

Quoted 16 lines

Cheers, Andreas

Signature · 10 lines
Andreas Dilger

Lustre Principal Architect
Intel High Performance Data Division


_______________________________________________
lustre-devel mailing list
[email protected]
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

Cheers, Andreas

Signature · 4 lines
Andreas Dilger

Lustre Principal Architect
Intel High Performance Data Division