Skip to contentSkip to conversation

target: split out helper for cxn timeout error stashing

PATCH In review David Disseldorp 2 participants 3 messages Oct 12, 2018 – Oct 13, 2018 SCSI target

lore
Share & tools
lore

David Disseldorp

author
#0 lore raw

PATCH v2 4/5 target: split out helper for cxn timeout error stashing

Replace existing nested code blocks with helper function calls.

Signed-off-by David Disseldorp <[email protected]>
---
 drivers/target/iscsi/iscsi_target_erl0.c | 15 +-------------
 drivers/target/iscsi/iscsi_target_util.c | 35 +++++++++++++++++---------------
 drivers/target/iscsi/iscsi_target_util.h |  1 +
 3 files changed, 21 insertions(+), 30 deletions(-)
drivers/target/iscsi/iscsi_target_erl0.c +1 -13
index 718fe9a1b709..1193cf884a28 100644
@@ -770,21 +770,8 @@ void iscsit_handle_time2retain_timeout(struct timer_list *t)
 
 	pr_err("Time2Retain timer expired for SID: %u, cleaning up"
 			" iSCSI session.\n", sess->sid);
-	{
-	struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
-
-	if (tiqn) {
-		spin_lock(&tiqn->sess_err_stats.lock);
-		strcpy(tiqn->sess_err_stats.last_sess_fail_rem_name,
-			(void *)sess->sess_ops->InitiatorName);
-		tiqn->sess_err_stats.last_sess_failure_type -				ISCSI_SESS_ERR_CXN_TIMEOUT;
-		tiqn->sess_err_stats.cxn_timeout_errors++;
-		atomic_long_inc(&sess->conn_timeout_errors);
-		spin_unlock(&tiqn->sess_err_stats.lock);
-	}
-	}
 
+	iscsit_fill_cxn_timeout_err_stats(sess);
 	spin_unlock_bh(&se_tpg->session_lock);
 	iscsit_close_session(sess);
 }
drivers/target/iscsi/iscsi_target_util.c +18 -15
index 931c51f56435..eacc9d5f78b7 100644
@@ -933,22 +933,7 @@ void iscsit_handle_nopin_response_timeout(struct timer_list *t)
 	conn->nopin_response_timer_flags &= ~ISCSI_TF_RUNNING;
 	spin_unlock_bh(&conn->nopin_timer_lock);
 
-	{
-	struct iscsi_portal_group *tpg = conn->sess->tpg;
-	struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
-
-	if (tiqn) {
-		spin_lock_bh(&tiqn->sess_err_stats.lock);
-		strcpy(tiqn->sess_err_stats.last_sess_fail_rem_name,
-				conn->sess->sess_ops->InitiatorName);
-		tiqn->sess_err_stats.last_sess_failure_type -				ISCSI_SESS_ERR_CXN_TIMEOUT;
-		tiqn->sess_err_stats.cxn_timeout_errors++;
-		atomic_long_inc(&conn->sess->conn_timeout_errors);
-		spin_unlock_bh(&tiqn->sess_err_stats.lock);
-	}
-	}
-
+	iscsit_fill_cxn_timeout_err_stats(sess);
 	iscsit_cause_connection_reinstatement(conn, 0);
 	iscsit_dec_conn_usage_count(conn);
 }

@@ -1407,3 +1392,21 @@ struct iscsi_tiqn *iscsit_snmp_get_tiqn(struct iscsi_conn *conn)
 
 	return tpg->tpg_tiqn;
 }
+
+void iscsit_fill_cxn_timeout_err_stats(struct iscsi_session *sess)
+{
+	struct iscsi_portal_group *tpg = sess->tpg;
+	struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
+
+	if (!tiqn)
+		return;
+
+	spin_lock_bh(&tiqn->sess_err_stats.lock);
+	strcpy(tiqn->sess_err_stats.last_sess_fail_rem_name,
+			sess->sess_ops->InitiatorName);
+	tiqn->sess_err_stats.last_sess_failure_type +			ISCSI_SESS_ERR_CXN_TIMEOUT;
+	tiqn->sess_err_stats.cxn_timeout_errors++;
+	atomic_long_inc(&sess->conn_timeout_errors);
+	spin_unlock_bh(&tiqn->sess_err_stats.lock);
+}
drivers/target/iscsi/iscsi_target_util.h +1 -0
index d66dfc212624..68e84803b0a1 100644
@@ -67,5 +67,6 @@ extern int rx_data(struct iscsi_conn *, struct kvec *, int, int);
 extern int tx_data(struct iscsi_conn *, struct kvec *, int, int);
 extern void iscsit_collect_login_stats(struct iscsi_conn *, u8, u8);
 extern struct iscsi_tiqn *iscsit_snmp_get_tiqn(struct iscsi_conn *);
+extern void iscsit_fill_cxn_timeout_err_stats(struct iscsi_session *);
 
 #endif /*** ISCSI_TARGET_UTIL_H ***/
Signature · 1 line
2.13.7
#1 lore raw

On Fri, 2018-10-12 at 12:01 +-0200, David Disseldorp wrote:
+AD4 +-void iscsit+AF8-fill+AF8-cxn+AF8-timeout+AF8-err+AF8-stats(struct iscsi+AF8-session +ACo-sess) +AD4 +-+AHs
+AD4 +- struct iscsi+AF8-portal+AF8-group +ACo-tpg +AD0 sess-+AD4-tpg+ADs
+AD4 +- struct iscsi+AF8-tiqn +ACo-tiqn +AD0 tpg-+AD4-tpg+AF8-tiqn+ADs
+AD4 +-
+AD4 +- if (+ACE-tiqn)
+AD4 +- return+ADs
+AD4 +-
+AD4 +- spin+AF8-lock+AF8-bh(+ACY-tiqn-+AD4-sess+AF8-err+AF8-stats.lock)+ADs
+AD4 +- strcpy(tiqn-+AD4-sess+AF8-err+AF8-stats.last+AF8-sess+AF8-fail+AF8-rem+AF8-name,
+AD4 +- sess-+AD4-sess+AF8-ops-+AD4-InitiatorName)+ADs

There have been too many problems with strcpy() and buffer overflows in the past. If the source and destination strings both have the same size, please add a BUILD+AF8-BUG+AF8-ON() statement that verifies that at compile time. If that not's the case, how about using strlcpy() to make it easy for anyone who reads the source code that no output buffer overflow will occur?

Thanks,

Bart.

author
#2 lore raw
In reply to David Disseldorp
Quoted 5 lines · Bart Van Assche

There have been too many problems with strcpy() and buffer overflows in the past. If the source and destination strings both have the same size, please add a BUILD_BUG_ON() statement that verifies that at compile time. If that not's the case, how about using strlcpy() to make it easy for anyone who reads the source code that no output buffer overflow will occur?

Both arrays are the same size (ISCSI_IQN_LEN). I'll change this over to use strlcpy(), as I agree that it helps readability.

Cheers, David