Skip to contentSkip to conversation

target: Adjustments for four function implementations

PATCH New SF Markus Elfring 1 participant 5 messages Nov 4, 2017

Subsystems (3)
SCSI Kernel janitors SCSI target
lore
Share & tools
lore

SF Markus Elfring

author
#0 lore raw

cover target: Adjustments for four function implementations

From: Markus Elfring <[email protected]>
Date: Sat, 4 Nov 2017 14:09:09 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
Use common error handling code in three functions
Combine two condition checks into one statement in target_xcopy_do_work() Improve a size determination in three functions
Delete an error message for a failed memory allocation in two functions

 drivers/target/target_core_xcopy.c | 69 ++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 36 deletions(-)
Signature · 1 line
2.15.0
author
#1 lore raw

PATCH 1/4 target: Use common error handling code in three functions

From: Markus Elfring <[email protected]>
Date: Sat, 4 Nov 2017 13:13:20 +0100

Adjust jump targets so that a bit of exception handling can be better reused at the end of these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/target/target_core_xcopy.c | 49 +++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 24 deletions(-)
drivers/target/target_core_xcopy.c +25 -24
index 9ee89e00cd77..b06877c57765 100644
@@ -701,11 +701,8 @@ static int target_xcopy_read_source(
 
 	rc = target_xcopy_setup_pt_cmd(xpt_cmd, xop, src_dev, &cdb[0],
 				remote_port, true);
-	if (rc < 0) {
-		ec_cmd->scsi_status = xpt_cmd->se_cmd.scsi_status;
-		transport_generic_free_cmd(se_cmd, 0);
-		return rc;
-	}
+	if (rc < 0)
+		goto set_status;
 
 	xop->xop_data_sg = se_cmd->t_data_sg;
 	xop->xop_data_nents = se_cmd->t_data_nents;

@@ -713,11 +710,9 @@ static int target_xcopy_read_source(
 		" memory\n", xop->xop_data_sg, xop->xop_data_nents);
 
 	rc = target_xcopy_issue_pt_cmd(xpt_cmd);
-	if (rc < 0) {
-		ec_cmd->scsi_status = xpt_cmd->se_cmd.scsi_status;
-		transport_generic_free_cmd(se_cmd, 0);
-		return rc;
-	}
+	if (rc < 0)
+		goto set_status;
+
 	/*
 	 * Clear off the allocated t_data_sg, that has been saved for
 	 * zero-copy WRITE submission reuse in struct xcopy_op..

@@ -726,6 +721,11 @@ static int target_xcopy_read_source(
 	se_cmd->t_data_nents = 0;
 
 	return 0;
+
+set_status:
+	ec_cmd->scsi_status = xpt_cmd->se_cmd.scsi_status;
+	transport_generic_free_cmd(se_cmd, 0);
+	return rc;
 }
 
 static int target_xcopy_write_destination(

@@ -775,19 +775,21 @@ static int target_xcopy_write_destination(
 		src_cmd->t_data_sg = xop->xop_data_sg;
 		src_cmd->t_data_nents = xop->xop_data_nents;
 
-		transport_generic_free_cmd(se_cmd, 0);
-		return rc;
+		goto err_free_cmd;
 	}
 
 	rc = target_xcopy_issue_pt_cmd(xpt_cmd);
 	if (rc < 0) {
 		ec_cmd->scsi_status = xpt_cmd->se_cmd.scsi_status;
 		se_cmd->se_cmd_flags &= ~SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC;
-		transport_generic_free_cmd(se_cmd, 0);
-		return rc;
+		goto err_free_cmd;
 	}
 
 	return 0;
+
+err_free_cmd:
+	transport_generic_free_cmd(se_cmd, 0);
+	return rc;
 }
 
 static void target_xcopy_do_work(struct work_struct *work)

@@ -801,10 +803,10 @@ static void target_xcopy_do_work(struct work_struct *work)
 	unsigned short nolb, cur_nolb, max_nolb, copied_nolb = 0;
 
 	if (target_parse_xcopy_cmd(xop) != TCM_NO_SENSE)
-		goto err_free;
+		goto err_free_xop;
 
 	if (WARN_ON_ONCE(!xop->src_dev) || WARN_ON_ONCE(!xop->dst_dev))
-		goto err_free;
+		goto err_free_xop;
 
 	src_dev = xop->src_dev;
 	dst_dev = xop->dst_dev;

@@ -835,7 +837,7 @@ static void target_xcopy_do_work(struct work_struct *work)
 
 		rc = target_xcopy_read_source(ec_cmd, xop, src_dev, src_lba, cur_nolb);
 		if (rc < 0)
-			goto out;
+			goto err_undepend_device;
 
 		src_lba += cur_nolb;
 		pr_debug("target_xcopy_do_work: Incremented READ src_lba to %llu\n",

@@ -846,10 +848,8 @@ static void target_xcopy_do_work(struct work_struct *work)
 
 		rc = target_xcopy_write_destination(ec_cmd, xop, dst_dev,
 						dst_lba, cur_nolb);
-		if (rc < 0) {
-			transport_generic_free_cmd(&xop->src_pt_cmd->se_cmd, 0);
-			goto out;
-		}
+		if (rc < 0)
+			goto err_free_cmd;
 
 		dst_lba += cur_nolb;
 		pr_debug("target_xcopy_do_work: Incremented WRITE dst_lba to %llu\n",

@@ -876,10 +876,11 @@ static void target_xcopy_do_work(struct work_struct *work)
 	target_complete_cmd(ec_cmd, SAM_STAT_GOOD);
 	return;
 
-out:
+err_free_cmd:
+	transport_generic_free_cmd(&xop->src_pt_cmd->se_cmd, 0);
+err_undepend_device:
 	xcopy_pt_undepend_remotedev(xop);
-
-err_free:
+err_free_xop:
 	kfree(xop);
 	/*
 	 * Don't override an error scsi status if it has already been set
Signature · 1 line
2.15.0
author
#2 lore raw

PATCH 2/4 target: Combine two condition checks into one statement in target_xcopy_do_work()

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Sat, 4 Nov 2017 13:26:46 +0100

The same label was used by goto statements after two condition checks. Thus use a single statement instead.

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/target/target_core_xcopy.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/target/target_core_xcopy.c +2 -4
index b06877c57765..46cac36014df 100644
@@ -802,10 +802,8 @@ static void target_xcopy_do_work(struct work_struct *work)
 	int rc = 0;
 	unsigned short nolb, cur_nolb, max_nolb, copied_nolb = 0;
 
-	if (target_parse_xcopy_cmd(xop) != TCM_NO_SENSE)
-		goto err_free_xop;
-
-	if (WARN_ON_ONCE(!xop->src_dev) || WARN_ON_ONCE(!xop->dst_dev))
+	if (target_parse_xcopy_cmd(xop) != TCM_NO_SENSE ||
+	    WARN_ON_ONCE(!xop->src_dev) || WARN_ON_ONCE(!xop->dst_dev))
 		goto err_free_xop;
 
 	src_dev = xop->src_dev;
Signature · 1 line
2.15.0
author
#3 lore raw

PATCH 3/4 target: Improve a size determination in three functions

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Sat, 4 Nov 2017 13:35:08 +0100

Replace the specification of data structures by pointer dereferences as the parameter for the operator "sizeof" to make the corresponding size determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/target/target_core_xcopy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/target/target_core_xcopy.c +3 -3
index 46cac36014df..1519e399392b 100644
@@ -680,7 +680,7 @@ static int target_xcopy_read_source(
 	unsigned char cdb[16];
 	bool remote_port = (xop->op_origin = XCOL_DEST_RECV_OP);
 
-	xpt_cmd = kzalloc(sizeof(struct xcopy_pt_cmd), GFP_KERNEL);
+	xpt_cmd = kzalloc(sizeof(*xpt_cmd), GFP_KERNEL);
 	if (!xpt_cmd) {
 		pr_err("Unable to allocate xcopy_pt_cmd\n");
 		return -ENOMEM;

@@ -742,7 +742,7 @@ static int target_xcopy_write_destination(
 	unsigned char cdb[16];
 	bool remote_port = (xop->op_origin = XCOL_SOURCE_RECV_OP);
 
-	xpt_cmd = kzalloc(sizeof(struct xcopy_pt_cmd), GFP_KERNEL);
+	xpt_cmd = kzalloc(sizeof(*xpt_cmd), GFP_KERNEL);
 	if (!xpt_cmd) {
 		pr_err("Unable to allocate xcopy_pt_cmd\n");
 		return -ENOMEM;

@@ -1010,7 +1010,7 @@ sense_reason_t target_do_xcopy(struct se_cmd *se_cmd)
 		return TCM_PARAMETER_LIST_LENGTH_ERROR;
 	}
 
-	xop = kzalloc(sizeof(struct xcopy_op), GFP_KERNEL);
+	xop = kzalloc(sizeof(*xop), GFP_KERNEL);
 	if (!xop)
 		goto err;
 	xop->xop_se_cmd = se_cmd;
Signature · 1 line
2.15.0
author
#4 lore raw

PATCH 4/4 target: Delete an error message for a failed memory allocation in two functions

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Sat, 4 Nov 2017 13:43:22 +0100

Omit an extra message for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/target/target_core_xcopy.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/target/target_core_xcopy.c +4 -6
index 1519e399392b..c2b3d3158208 100644
@@ -681,10 +681,9 @@ static int target_xcopy_read_source(
 	bool remote_port = (xop->op_origin = XCOL_DEST_RECV_OP);
 
 	xpt_cmd = kzalloc(sizeof(*xpt_cmd), GFP_KERNEL);
-	if (!xpt_cmd) {
-		pr_err("Unable to allocate xcopy_pt_cmd\n");
+	if (!xpt_cmd)
 		return -ENOMEM;
-	}
+
 	init_completion(&xpt_cmd->xpt_passthrough_sem);
 	se_cmd = &xpt_cmd->se_cmd;
 

@@ -743,10 +742,9 @@ static int target_xcopy_write_destination(
 	bool remote_port = (xop->op_origin = XCOL_SOURCE_RECV_OP);
 
 	xpt_cmd = kzalloc(sizeof(*xpt_cmd), GFP_KERNEL);
-	if (!xpt_cmd) {
-		pr_err("Unable to allocate xcopy_pt_cmd\n");
+	if (!xpt_cmd)
 		return -ENOMEM;
-	}
+
 	init_completion(&xpt_cmd->xpt_passthrough_sem);
 	se_cmd = &xpt_cmd->se_cmd;
Signature · 1 line
2.15.0