From 346fb947cb46ee8afcf5b2205e40af4ce8d29a79 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 1 Oct 2018 13:22:36 +0200 Subject: [PATCH 1/6] iser_rcv_completion: unify error handling Move the iscsi_set_error to iser_post_recv, and avoid leaking the input buffer "in". Signed-off-by: Paolo Bonzini --- lib/iser.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/iser.c b/lib/iser.c index 898d6b5..ef3957e 100644 --- a/lib/iser.c +++ b/lib/iser.c @@ -964,7 +964,7 @@ iser_post_recvm(struct iser_conn *iser_conn, int count) iser_conn->post_recv_buf_count += count; ret = ibv_post_recv(iser_conn->qp, iser_conn->rx_wr, &rx_wr_failed); if (ret) { - iscsi_set_error(iscsi, "ib_post_recv failed ret=%d", ret); + iscsi_set_error(iscsi, "posting %d rx bufs, ib_post_recv failed ret=%d", count, ret); iser_conn->post_recv_buf_count -= count; } else iser_conn->rx_desc_head = my_rx_head; @@ -1037,12 +1037,13 @@ iser_rcv_completion(struct iser_rx_desc *rx_desc, if (iscsi->session_type == ISCSI_SESSION_NORMAL) { if(iser_alloc_rx_descriptors(iser_conn,255)) { iscsi_set_error(iscsi, "iser_alloc_rx_descriptors Failed\n"); - return -1; + err = -1; + goto error; } err = iser_post_recvm(iser_conn, ISER_MIN_POSTED_RX); if (err) { - iscsi_set_error(iscsi, "posting %d rx bufs err %d", count, err); - return -1; + err = -1; + goto error; } } in->hdr = (unsigned char*)rx_desc->iscsi_header; @@ -1089,7 +1090,6 @@ iser_rcv_completion(struct iser_rx_desc *rx_desc, ISCSI_LIST_ADD_END(&iser_conn->tx_desc, iser_pdu->desc); nop_target: - /* decrementing conn->post_recv_buf_count only --after-- freeing the * * task eliminates the need to worry on tasks which are completed in * * parallel to the execution of iser_conn_term. So the code that waits * @@ -1107,16 +1107,16 @@ nop_target: count = iser_conn->qp_max_recv_dtos - outstanding; err = iser_post_recvm(iser_conn, count); if (err) { - iscsi_set_error(iscsi, "posting %d rx bufs err %d", count, err); - return -1; + err = -1; + goto error; } } receive: - err = iscsi_process_pdu(iscsi, in); - iscsi_free(iscsi, in); +error: + iscsi_free(iscsi, in); return err; } From f0fcee72c477dea57f0a50887b7368ba6d4ec33b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 1 Oct 2018 13:35:14 +0200 Subject: [PATCH 2/6] iser: fix posting of receive descriptors The old code is effectively always posting iser_conn->min_posted_rx descriptors, since it is if (outstanding + iser_conn->min_posted_rx <= iser_conn->qp_max_recv_dtos) { if(iser_conn->qp_max_recv_dtos - outstanding > iser_conn->min_posted_rx) count = iser_conn->min_posted_rx; else count = iser_conn->qp_max_recv_dtos - outstanding; which is equivalent to if(iser_conn->qp_max_recv_dtos - outstanding >= iser_conn->min_posted_rx) if(iser_conn->qp_max_recv_dtos - outstanding > iser_conn->min_posted_rx) count = iser_conn->min_posted_rx; else count = iser_conn->min_posted_rx; So the "if" is redundant and the "min_posted_rx" is actually behaving more like a _maximum_ number of posted descriptors in one iser_post_recvm. Fix it with the (presumably) intended logic and remove a goto along the way. Signed-off-by: Paolo Bonzini --- lib/iser.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/lib/iser.c b/lib/iser.c index ef3957e..d140ad6 100644 --- a/lib/iser.c +++ b/lib/iser.c @@ -1028,7 +1028,7 @@ iser_rcv_completion(struct iser_rx_desc *rx_desc, struct iser_conn *iser_conn) { struct iscsi_in_pdu *in = NULL; - int outstanding, count = 0, err; + int empty, err; struct iscsi_context *iscsi = iser_conn->cma_id->context; in = iscsi_malloc(iscsi, sizeof(*in)); @@ -1096,23 +1096,17 @@ nop_target: * for the posted rx bufs refcount to become zero handles everything */ iser_conn->post_recv_buf_count--; - if ((unsigned char *)rx_desc == iser_conn->login_resp_buf) - goto receive; - - outstanding = iser_conn->post_recv_buf_count; - if (outstanding + iser_conn->min_posted_rx <= iser_conn->qp_max_recv_dtos) { - if(iser_conn->qp_max_recv_dtos - outstanding > iser_conn->min_posted_rx) - count = iser_conn->min_posted_rx; - else - count = iser_conn->qp_max_recv_dtos - outstanding; - err = iser_post_recvm(iser_conn, count); - if (err) { - err = -1; - goto error; + if ((unsigned char *)rx_desc != iser_conn->login_resp_buf) { + empty = iser_conn->qp_max_recv_dtos - iser_conn->post_recv_buf_count; + if (empty >= iser_conn->min_posted_rx) { + err = iser_post_recvm(iser_conn, empty); + if (err) { + err = -1; + goto error; + } } } -receive: err = iscsi_process_pdu(iscsi, in); error: From f507c94774bff0c851880813d95f5d0a8fbdace8 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 1 Oct 2018 13:40:51 +0200 Subject: [PATCH 3/6] sync: remove unnecessary checks state is always non-NULL in iscsi_sync_cb and iscsi_discovery_cb. Signed-off-by: Paolo Bonzini --- lib/sync.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/sync.c b/lib/sync.c index f8a139c..0ceb0cb 100644 --- a/lib/sync.c +++ b/lib/sync.c @@ -88,10 +88,8 @@ iscsi_sync_cb(struct iscsi_context *iscsi _U_, int status, { struct iscsi_sync_state *state = private_data; - if (state != NULL) { - state->status = status; - state->finished = 1; - } + state->status = status; + state->finished = 1; } int @@ -1871,11 +1869,9 @@ iscsi_discovery_cb(struct iscsi_context *iscsi _U_, int status, } } - if (state != NULL) { - state->status = status; - state->finished = 1; - state->ptr = dahead; - } + state->status = status; + state->finished = 1; + state->ptr = dahead; } struct iscsi_discovery_address * From 4728d4eaa605a02cfc767dd40e5436c670e88b0f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 1 Oct 2018 14:13:15 +0200 Subject: [PATCH 4/6] do not warn for strncpy strncpy use in iscsi_reconnect is just fine. Do not warn for it, and also do not warn if clang does not recognize the flag. Signed-off-by: Paolo Bonzini --- configure.ac | 1 + 1 file changed, 1 insertion(+) diff --git a/configure.ac b/configure.ac index cf62b64..4001400 100644 --- a/configure.ac +++ b/configure.ac @@ -29,6 +29,7 @@ AC_ARG_ENABLE([werror], [AS_HELP_STRING([--disable-werror], if test "$ac_cv_prog_gcc" = yes; then WARN_CFLAGS="-Wall -W -Wshadow -Wstrict-prototypes -Wpointer-arith -Wcast-align -Wno-strict-aliasing" + WARN_CFLAGS="$WARN_CFLAGS -Wno-unknown-warning-option -Wno-stringop-truncation" if test "x$enable_werror" != "xno"; then WARN_CFLAGS="$WARN_CFLAGS -Werror" fi From 679d0abe7c142df178a907397551c4d9695cc667 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 1 Oct 2018 14:14:24 +0200 Subject: [PATCH 5/6] avoid fallthrough --- lib/scsi-lowlevel.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/scsi-lowlevel.c b/lib/scsi-lowlevel.c index 5ddd709..747ce0c 100644 --- a/lib/scsi-lowlevel.c +++ b/lib/scsi-lowlevel.c @@ -1086,6 +1086,7 @@ scsi_maintenancein_datain_getfullsize(struct scsi_task *task) (task_get_uint8(task, 1) & 0x80) ? 12 : 0 + task_get_uint16(task, 2); } + return -1; default: return -1; } From bffafc1c3003c2ee05d28eaa345e5854bc36014d Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 1 Oct 2018 14:16:14 +0200 Subject: [PATCH 6/6] avoid truncation when logging message that includes target name --- lib/logging.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/logging.c b/lib/logging.c index be518fc..61c7440 100644 --- a/lib/logging.c +++ b/lib/logging.c @@ -73,9 +73,9 @@ iscsi_log_message(struct iscsi_context *iscsi, int level, const char *format, .. } if (iscsi->target_name[0]) { - static char message2[1024]; + static char message2[1282]; - snprintf(message2, 1024, "%s [%s]", message, iscsi->target_name); + snprintf(message2, 1282, "%s [%s]", message, iscsi->target_name); iscsi->log_fn(level, message2); } else