From dbc78782e188d65c853a14e467ac29ed744adfc2 Mon Sep 17 00:00:00 2001 From: Gleb Chesnokov Date: Wed, 21 Oct 2026 13:26:49 +0300 Subject: [PATCH] iscsi-scstd: Clarify result handling and connection cleanup Checkpatch reports: ASSIGN_IN_IF: do not use assignment in if condition DEEP_INDENTATION: Too many leading tabs - consider code refactoring USE_NEGATIVE_ERRNO: return of an errno should typically be negative (ie: return -EAGAIN) Separate status assignments from simple if conditions and preserve short-circuit dispatch bounds. Leave the parameter-dispatch else-if chains and their lookup precedence intact. Flatten incoming-connection cleanup with early continues. Keep the CLOSE case, kernel-owned destruction, session-count rollback and empty-session release in their original order; no new ownership transfer is introduced. Return -EAGAIN for an empty nonblocking event queue. The drain loop checks zero versus nonzero, while main-loop callers discard this result. Retain EINTR retry and event draining behavior and document the return convention. --- iscsi-scst/usr/event.c | 12 ++++--- iscsi-scst/usr/iscsi_adm.c | 7 ++-- iscsi-scst/usr/iscsi_scstd.c | 62 ++++++++++++++++++------------------ iscsi-scst/usr/iscsid.c | 3 +- 4 files changed, 45 insertions(+), 39 deletions(-) diff --git a/iscsi-scst/usr/event.c b/iscsi-scst/usr/event.c index bf3292c18..743781cf4 100644 --- a/iscsi-scst/usr/event.c +++ b/iscsi-scst/usr/event.c @@ -520,7 +520,8 @@ static int handle_e_get_attr_value(int fd, const struct iscsi_kern_event *event) p = buf; pp = config_sep_string(&p); - if (!((idx = params_index_by_name(pp, target_keys)) < 0)) { + idx = params_index_by_name(pp, target_keys); + if (idx >= 0) { if (target == NULL) { log_error("Target expected for attr %s", pp); res = -EINVAL; @@ -850,7 +851,8 @@ static int handle_e_set_attr_value(int fd, const struct iscsi_kern_event *event) p = buf; pp = config_sep_string(&p); - if (!((idx = params_index_by_name(pp, target_keys)) < 0)) { + idx = params_index_by_name(pp, target_keys); + if (idx >= 0) { struct iscsi_param params[target_key_last]; struct session *session; @@ -1184,6 +1186,7 @@ out_free_server: } +/* Return zero after handling an event, or -EAGAIN when no event is available. */ int handle_iscsi_events(int fd, bool wait) { struct session *session; @@ -1200,9 +1203,10 @@ int handle_iscsi_events(int fd, bool wait) STATIC_ASSERT(sizeof(event) % NLMSG_ALIGNTO == 0); retry: - if ((rc = nl_read(fd, &event, sizeof(event), wait)) < 0) { + rc = nl_read(fd, &event, sizeof(event), wait); + if (rc < 0) { if (errno == EAGAIN) - return EAGAIN; + return -EAGAIN; if (errno == EINTR) goto retry; log_error("read netlink fd (%d) failed: %s", fd, strerror(errno)); diff --git a/iscsi-scst/usr/iscsi_adm.c b/iscsi-scst/usr/iscsi_adm.c index de5248bd7..febb0b9eb 100644 --- a/iscsi-scst/usr/iscsi_adm.c +++ b/iscsi-scst/usr/iscsi_adm.c @@ -334,9 +334,9 @@ static int trgt_handle(int op, u32 set, u32 tid, char *params) switch (op) { case OP_NEW: { - char *p = params; + char *p = params ? strchr(params, '=') : NULL; - if (!params || !(p = strchr(params, '='))) { + if (!p) { fprintf(stderr, "Target name required\n"); err = -EINVAL; goto out; @@ -356,7 +356,8 @@ static int trgt_handle(int op, u32 set, u32 tid, char *params) break; case OP_UPDATE: req.rcmnd = C_TRGT_UPDATE; - if ((err = parse_trgt_params(&req.u.trgt, params)) < 0) + err = parse_trgt_params(&req.u.trgt, params); + if (err < 0) goto out; break; case OP_SHOW: diff --git a/iscsi-scst/usr/iscsi_scstd.c b/iscsi-scst/usr/iscsi_scstd.c index f9b37f644..dd1a9040e 100644 --- a/iscsi-scst/usr/iscsi_scstd.c +++ b/iscsi-scst/usr/iscsi_scstd.c @@ -760,6 +760,7 @@ static void event_loop(void) for (i = 0; i < INCOMING_MAX; i++) { struct connection *conn = incoming[i]; struct pollfd *pollfd = &poll_array[POLL_INCOMING + i]; + struct session *sess; if (!conn || !pollfd->revents) continue; @@ -768,39 +769,38 @@ static void event_loop(void) event_conn(conn, pollfd); - if ((conn->state == STATE_CLOSE) || - (conn->state == STATE_EXIT) || - (conn->state == STATE_DROP)) { - struct session *sess = conn->sess; + if (conn->state != STATE_CLOSE && + conn->state != STATE_EXIT && + conn->state != STATE_DROP) + continue; - log_debug(1, "closing conn %p state=0x%x fd=%u", - conn, conn->state, pollfd->fd); - conn_free_pdu(conn); - close(pollfd->fd); - pollfd->fd = -1; - incoming[i] = NULL; - incoming_cnt--; - if (conn->state != STATE_CLOSE) { - if (conn->passed_to_kern) { - kernel_conn_destroy(conn->tid, - conn->sess->sid.id64, - conn->cid); - } else { - /* - * Check if session could not be established, - * but sessions count was already incremented - */ - if (!sess && conn->sessions_count_incremented) - conn->target->sessions_count--; - log_debug(1, "conn %p freed (sess %p, empty %d)", - conn, sess, - sess ? list_empty(&sess->conn_list) : -1); - conn_free(conn); - if (sess && list_empty(&sess->conn_list)) - session_free(sess); - } - } + sess = conn->sess; + log_debug(1, "closing conn %p state=0x%x fd=%u", + conn, conn->state, pollfd->fd); + conn_free_pdu(conn); + close(pollfd->fd); + pollfd->fd = -1; + incoming[i] = NULL; + incoming_cnt--; + if (conn->state == STATE_CLOSE) + continue; + + if (conn->passed_to_kern) { + kernel_conn_destroy(conn->tid, conn->sess->sid.id64, conn->cid); + continue; } + + /* + * The count may have been incremented before session + * establishment failed. + */ + if (!sess && conn->sessions_count_incremented) + conn->target->sessions_count--; + log_debug(1, "conn %p freed (sess %p, empty %d)", + conn, sess, sess ? list_empty(&sess->conn_list) : -1); + conn_free(conn); + if (sess && list_empty(&sess->conn_list)) + session_free(sess); } } } diff --git a/iscsi-scst/usr/iscsid.c b/iscsi-scst/usr/iscsid.c index d3fb72796..a8dd41ea7 100644 --- a/iscsi-scst/usr/iscsid.c +++ b/iscsi-scst/usr/iscsid.c @@ -1162,7 +1162,8 @@ static void cmnd_exec_login(struct connection *conn) switch (conn->state) { case STATE_SECURITY: case STATE_SECURITY_DONE: - if ((nsg_disagree = text_check_params(conn))) { + nsg_disagree = text_check_params(conn); + if (nsg_disagree) { conn->state = STATE_LOGIN; nsg = ISCSI_FLG_NSG_LOGIN; break;