On Fri, 2013-03-22 at 10:23 -0700, Andy Grover wrote:
> On 03/07/2013 05:45 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> >
> > This patch performs the initial conversion of existing traditional iscsi
> > to use iscsit_transport API callers. This includes:
> >
> > - iscsi-np cleanups for iscsit_transport_type
> > - Add iscsi-np transport calls w/ ->iscsit_setup_up() and ->iscsit_free_np()
> > - Convert login thread process context to use ->iscsit_accept_np() for
> > connections with pre-allocated struct iscsi_conn
> > - Convert existing socket accept code to iscsit_accept_np()
> > - Convert login RX/TX callers to use ->iscsit_get_login_rx() and
> > ->iscsit_put_login_tx() to exchange request/response PDUs
> > - Convert existing socket login RX/TX calls into iscsit_get_login_rx()
> > and iscsit_put_login_tx()
> > - Change iscsit_close_connection() to invoke ->iscsit_free_conn() +
> > iscsit_put_transport() calls.
> > - Add iscsit_create_transport() + iscsit_destroy_transport() calls
> > to module init/exit
> >
> > Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> > ---
> > drivers/target/iscsi/iscsi_target.c | 35 ++-
> > drivers/target/iscsi/iscsi_target_core.h | 15 +-
> > drivers/target/iscsi/iscsi_target_login.c | 411 ++++++++++++++++--------
> > drivers/target/iscsi/iscsi_target_login.h | 6 +
> > drivers/target/iscsi/iscsi_target_nego.c | 185 ++----------
> > drivers/target/iscsi/iscsi_target_nego.h | 11 +-
> > drivers/target/iscsi/iscsi_target_parameters.c | 12 +-
> > drivers/target/iscsi/iscsi_target_tpg.c | 6 +-
> > drivers/target/iscsi/iscsi_target_util.c | 27 +--
> > 9 files changed, 376 insertions(+), 332 deletions(-)
> >
> > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> > index 23a98e6..4dc1c9b 100644
> > --- a/drivers/target/iscsi/iscsi_target.c
> > +++ b/drivers/target/iscsi/iscsi_target.c
<SNIP>
> > @@ -4045,6 +4060,12 @@ int iscsit_close_connection(
> >
> > if (conn->sock)
> > sock_release(conn->sock);
> > +
> > + if (conn->conn_transport->iscsit_free_conn)
> > + conn->conn_transport->iscsit_free_conn(conn);
>
> For all the function pointers in conn_transport, won't they always be
> there? If so, can we just call them w/o checking?
For this case, no. ->iscsit_free_conn() is currently present for only
ib_isert.ko code.
> > diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> > index fdb632f..9354a5f 100644
> > --- a/drivers/target/iscsi/iscsi_target_login.c
> > +++ b/drivers/target/iscsi/iscsi_target_login.c
<SNIP>
> > @@ -635,7 +677,13 @@ static int iscsi_post_login_handler(
> > spin_unlock_bh(&sess->conn_lock);
> >
> > iscsi_post_login_start_timers(conn);
> > - iscsi_activate_thread_set(conn, ts);
> > +
> > + if (conn->conn_transport == ISCSI_TCP) {
> > + iscsi_activate_thread_set(conn, ts);
>
> I thought conn_transport was a pointer? It looks like it's being
> compared against an enum here.
>
This has already been removed for RFC-v2 code.
> > + } else {
> > + printk("Not calling iscsi_activate_thread_set....\n");
> > + dump_stack();
>
> Left-in debug code?
Ditto here too..
>
> > +int iscsit_get_login_rx(struct iscsi_conn *conn, struct iscsi_login *login)
> > +{
> > + struct iscsi_login_req *login_req;
> > + u32 padding = 0, payload_length;
> > +
> > + if (iscsi_login_rx_data(conn, login->req, ISCSI_HDR_LEN) < 0)
> > + return -1;
> > +
> > + login_req = (struct iscsi_login_req *)login->req;
> > + payload_length = ntoh24(login_req->dlength);
> > + padding = ((-payload_length) & 3);
> > +
> > + pr_debug("Got Login Command, Flags 0x%02x, ITT: 0x%08x,"
> > + " CmdSN: 0x%08x, ExpStatSN: 0x%08x, CID: %hu, Length: %u\n",
> > + login_req->flags, login_req->itt, login_req->cmdsn,
> > + login_req->exp_statsn, login_req->cid, payload_length);
> > + /*
> > + * Setup the initial iscsi_login values from the leading
> > + * login request PDU.
> > + */
> > + if (login->first_request) {
> > + login_req = (struct iscsi_login_req *)login->req;
> > + login->leading_connection = (!login_req->tsih) ? 1 : 0;
> > + login->current_stage =
> > + (login_req->flags & ISCSI_FLAG_LOGIN_CURRENT_STAGE_MASK) >> 2;
> > + login->version_min = login_req->min_version;
> > + login->version_max = login_req->max_version;
> > + memcpy(login->isid, login_req->isid, 6);
> > + login->cmd_sn = be32_to_cpu(login_req->cmdsn);
> > + login->init_task_tag = login_req->itt;
> > + login->initial_exp_statsn = be32_to_cpu(login_req->exp_statsn);
> > + login->cid = be16_to_cpu(login_req->cid);
> > + login->tsih = be16_to_cpu(login_req->tsih);
> > + }
> > +
> > + if (iscsi_target_check_login_request(conn, login) < 0)
> > + return -1;
>
> Better return values?
>
In the failure case here, iscsit_tx_login_rsp() will have already been
called to queue the login response with the appropriate status class +
non-zero status detail.
> > @@ -737,7 +676,7 @@ static void iscsi_initiatorname_tolower(
> > /*
> > * Processes the first Login Request..
> > */
> > -static int iscsi_target_locate_portal(
> > +int iscsi_target_locate_portal(
> > struct iscsi_np *np,
> > struct iscsi_conn *conn,
> > struct iscsi_login *login)
> > @@ -746,29 +685,13 @@ static int iscsi_target_locate_portal(
> > char *tmpbuf, *start = NULL, *end = NULL, *key, *value;
> > struct iscsi_session *sess = conn->sess;
> > struct iscsi_tiqn *tiqn;
> > - struct iscsi_login_req *login_req;
> > + struct iscsi_login_req *login_req;
>
> tab got replaced by spaces?
>
Fixed.
Thanks,
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
