Message-Id: <1300829702.21880.338.camel@haakon2.linux-iscsi.org>
Date:	Tue, 22 Mar 2011 14:35:02 -0700
From:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:	Mike Christie <michaelc@...wisc.edu>
Cc:	linux-scsi <linux-scsi@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	James Bottomley <James.Bottomley@...senPartnership.com>,
	Christoph Hellwig <hch@....de>, Hannes Reinecke <hare@...e.de>,
	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
	Boaz Harrosh <bharrosh@...asas.com>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Douglas Gilbert <dgilbert@...erlog.com>,
	Jesper Juhl <jj@...osbits.net>
Subject: Re: [RFC-v4 11/12] iscsi-target: Add misc utility and debug logic

On Mon, 2011-03-21 at 23:04 -0500, Mike Christie wrote:
> On 03/20/2011 04:31 AM, Nicholas A. Bellinger wrote
> 
> > +
> > +/*
> > + *	Called with cmd->r2t_lock held.
> > + */
> > +void iscsit_free_r2t(struct iscsi_r2t *r2t, struct iscsi_cmd *cmd)
> > +{
> > +	list_del(&r2t->r2t_list);
> > +	kmem_cache_free(lio_r2t_cache, r2t);
> > +}
> > +
> > +void iscsit_free_r2ts_from_list(struct iscsi_cmd *cmd)
> > +{
> > +	struct iscsi_r2t *r2t, *r2t_tmp;
> > +
> > +	spin_lock_bh(&cmd->r2t_lock);
> > +	list_for_each_entry_safe(r2t, r2t_tmp,&cmd->cmd_r2t_list, r2t_list) {
> > +		list_del(&r2t->r2t_list);
> > +		kmem_cache_free(lio_r2t_cache, r2t);
> 
> I think that is iscsit_free_r2t()
> 

Fixed

> > +	}
> > +	spin_unlock_bh(&cmd->r2t_lock);
> > +}
> > +
> > +/*
> > + *	May be called from interrupt context.
> 
> 
> How does this get called from interrupt context? Is it a real interrupt 
> or a soft irq? I could not find the code path.
> 
> 
> 

This is actually software interrupt context usage from the NopIN timer
to queue up iscsi_cmd from iscsit_add_nopin()

Adding a comment here..

> 
> 
> > +
> > +struct iscsi_r2t *iscsit_get_holder_for_r2tsn(
> > +	struct iscsi_cmd *cmd,
> > +	u32 r2t_sn)
> > +{
> > +	struct iscsi_r2t *r2t;
> > +
> > +	spin_lock_bh(&cmd->r2t_lock);
> > +	list_for_each_entry(r2t,&cmd->cmd_r2t_list, r2t_list) {
> > +		if (r2t->r2t_sn == r2t_sn)
> > +			break;
> > +	}
> > +	spin_unlock_bh(&cmd->r2t_lock);
> > +
> > +	return (r2t) ? r2t : NULL;
> 
> Don't need "( ")".
> 

<nod>, fixing this to unlock and return on matching r2t or return NULL
otherwise

> 
> > +}
> > +
> > +#define SERIAL_BITS	31
> > +#define MAX_BOUND	(u32)2147483647UL
> > +
> > +static inline int serial_lt(u32 x, u32 y)
> > +{
> > +	return (x != y)&&  (((x<  y)&&  ((y - x)<  MAX_BOUND)) ||
> > +		((x>  y)&&  ((x - y)>  MAX_BOUND)));
> > +}
> > +
> > +static inline int serial_lte(u32 x, u32 y)
> > +{
> > +	return (x == y) ? 1 : serial_lt(x, y);
> > +}
> > +
> > +static inline int serial_gt(u32 x, u32 y)
> > +{
> > +	return (x != y)&&  (((x<  y)&&  ((y - x)>  MAX_BOUND)) ||
> > +		((x>  y)&&  ((x - y)<  MAX_BOUND)));
> > +}
> > +
> > +static inline int serial_gte(u32 x, u32 y)
> > +{
> > +	return (x == y) ? 1 : serial_gt(x, y);
> > +}
> 
> 
> You should merged these with libiscsi.c's iscsi_sna_* and put in 
> iscsi_proto.h.
> 
> I think this is not iscsi specific is it? It seems like it could go 
> someone more generic.
> 
> 

I think this makes sense.

Moveing libiscsi.c LT* and iscsi_target_util.c GT* usage into static
inlines in iscsi_proto.h, and dropped the internal usage here.

Will include these as a seperate iscsi patch for review.

> 
> > +void iscsit_release_cmd_direct(struct iscsi_cmd *cmd)
> > +{
> > +	iscsit_free_r2ts_from_list(cmd);
> > +	iscsit_free_all_datain_reqs(cmd);
> > +
> > +	kfree(cmd->buf_ptr);
> > +	kfree(cmd->pdu_list);
> > +	kfree(cmd->seq_list);
> > +	kfree(cmd->tmr_req);
> > +	kfree(cmd->iov_data);
> > +
> > +	kmem_cache_free(lio_cmd_cache, cmd);
> > +}
> > +
> > +void __iscsit_release_cmd_to_pool(struct iscsi_cmd *cmd, struct iscsi_session *sess)
> > +{
> > +	struct iscsi_conn *conn = cmd->conn;
> > +
> > +	iscsit_free_r2ts_from_list(cmd);
> > +	iscsit_free_all_datain_reqs(cmd);
> > +
> > +	kfree(cmd->buf_ptr);
> > +	kfree(cmd->pdu_list);
> > +	kfree(cmd->seq_list);
> > +	kfree(cmd->tmr_req);
> > +	kfree(cmd->iov_data);
> > +
> > +	if (conn) {
> > +		iscsit_remove_cmd_from_immediate_queue(cmd, conn);
> > +		iscsit_remove_cmd_from_response_queue(cmd, conn);
> > +	}
> > +
> > +	kmem_cache_free(lio_cmd_cache, cmd);
> > +}
> 
> sess is not used and I could not figure the _to_pool part of the name.
> 

The '_to_pool' is some left over cruft that still exists in
include/target/target_core_fabric_ops.h that needs to be fixed up in
target core as a seperate item and merged into a single
TFO->release_cmd().

> This is not some sort of mistake, right? iscsit_release_cmd_direct and 
> __iscsit_release_cmd_to_pool look alike except for the conn check 
> related code. Did you mean to merge those functions?
> 
> 

<nod>, now dropping iscsit_release_cmd_direct() and
iscsit_release_cmd_to_pool() usage for single iscsit_release_cmd() call.

> 
> > +
> > +void iscsit_release_cmd_to_pool(struct iscsi_cmd *cmd)
> > +{
> > +	if (!cmd->conn&&  !cmd->sess) {
> > +		iscsit_release_cmd_direct(cmd);
> > +	} else {
> > +		__iscsit_release_cmd_to_pool(cmd, (cmd->conn) ?
> > +			cmd->conn->sess : cmd->sess);
> > +	}
> > +}
> > +
> > +/*
> > + *	Routine to pack an ordinary (LINUX) LUN 32-bit number
> > + *		into an 8-byte LUN structure
> > + *	(see SAM-2, Section 4.12.3 page 39)
> > + *	Thanks to UNH for help with this :-).
> > + */
> > +inline u64 iscsit_pack_lun(unsigned int lun)
> > +{
> > +	u64	result;
> > +
> > +	result = ((lun&  0xff)<<  8);	/* LSB of lun into byte 1 big-endian */
> > +
> > +	if (0) {
> > +		/* use flat space addressing method, SAM-2 Section 4.12.4
> > +			-	high-order 2 bits of byte 0 are 01
> > +			-	low-order 6 bits of byte 0 are MSB of the lun
> > +			-	all 8 bits of byte 1 are LSB of the lun
> > +			-	all other bytes (2 thru 7) are 0
> > +		 */
> > +		result |= 0x40 | ((lun>>  8)&  0x3f);
> > +	}
> > +	/* else use peripheral device addressing method, Sam-2 Section 4.12.5
> > +			-	high-order 2 bits of byte 0 are 00
> > +			-	low-order 6 bits of byte 0 are all 0
> > +			-	all 8 bits of byte 1 are the lun
> > +			-	all other bytes (2 thru 7) are 0
> > +	*/
> > +
> > +	return cpu_to_le64(result);
> > +}
> 
> Is this int_to_scsilun?
> 
> 

Hmmm, this is actually still by target_core_fabric_ops->pack_lun()
for target_core_device.c:transport_core_report_lun_response().
Converted this in iscsi_target_mod and will provide a seperate patch for
target core to use int_to_scsilun() for REPORT_LUNs, and will remove the
the ->pack_lun() vector for mainline target in a seperate follow-up
patch.

> > +
> > +/*
> > + *	Routine to pack an 8-byte LUN structure into a ordinary (LINUX) 32-bit
> > + *	LUN number (see SAM-2, Section 4.12.3 page 39)
> > + *	Thanks to UNH for help with this :-).
> > + */
> > +inline u32 iscsit_unpack_lun(unsigned char *lun_ptr)
> > +{
> > +	u32	result, temp;
> > +
> > +	result = *(lun_ptr+1);  /* LSB of lun from byte 1 big-endian */
> > +
> > +	switch (temp = ((*lun_ptr)>>6)) { /* high 2 bits of byte 0 big-endian */
> > +	case 0: /* peripheral device addressing method, Sam-2 Section 4.12.5
> > +		-	high-order 2 bits of byte 0 are 00
> > +		-	low-order 6 bits of byte 0 are all 0
> > +		-	all 8 bits of byte 1 are the lun
> > +		-	all other bytes (2 thru 7) are 0
> > +		 */
> > +		if (*lun_ptr != 0) {
> > +			printk(KERN_ERR "Illegal Byte 0 in LUN peripheral"
> > +				" device addressing method %u, expected 0\n",
> > +				*lun_ptr);
> > +		}
> > +		break;
> > +	case 1: /* flat space addressing method, SAM-2 Section 4.12.4
> > +		-	high-order 2 bits of byte 0 are 01
> > +		-	low-order 6 bits of byte 0 are MSB of the lun
> > +		-	all 8 bits of byte 1 are LSB of the lun
> > +		-	all other bytes (2 thru 7) are 0
> > +		 */
> > +		result += ((*lun_ptr)&  0x3f)<<  8;
> > +		break;
> > +	default: /* (extended) logical unit addressing */
> > +		printk(KERN_ERR "Unimplemented LUN addressing method %u, "
> > +			"PDA method used instead\n", temp);
> > +		break;
> > +	}
> > +
> > +	return result;
> > +}
> 
> 
> scsilun_to_int?
> 
> 
> 

Converted all iscsi_unpack_lun() usage to scsilun_to_int().

> 
> 
> > +
> > +int iscsit_check_session_usage_count(struct iscsi_session *sess)
> > +{
> > +	spin_lock_bh(&sess->session_usage_lock);
> > +	if (atomic_read(&sess->session_usage_count)) {
> > +		atomic_set(&sess->session_waiting_on_uc, 1);
> > +		spin_unlock_bh(&sess->session_usage_lock);
> > +		if (in_interrupt())
> > +			return 2;
> > +
> > +		wait_for_completion(&sess->session_waiting_on_uc_comp);
> > +		return 1;
> > +	}
> > +	spin_unlock_bh(&sess->session_usage_lock);
> > +
> > +	return 0;
> > +}
> > +
> > +void iscsit_dec_session_usage_count(struct iscsi_session *sess)
> > +{
> > +	spin_lock_bh(&sess->session_usage_lock);
> > +	atomic_dec(&sess->session_usage_count);
> > +
> > +	if (!atomic_read(&sess->session_usage_count)&&
> > +	     atomic_read(&sess->session_waiting_on_uc))
> > +		complete(&sess->session_waiting_on_uc_comp);
> > +
> > +	spin_unlock_bh(&sess->session_usage_lock);
> > +}
> > +
> > +void iscsit_inc_session_usage_count(struct iscsi_session *sess)
> > +{
> > +	spin_lock_bh(&sess->session_usage_lock);
> > +	atomic_inc(&sess->session_usage_count);
> > +	spin_unlock_bh(&sess->session_usage_lock);
> > +}
> 
> 
> 
> It seems strange for the session_usage_count and waiting_on_uc to be 
> atomic and accessed under the spin lock. I think you normally do one or 
> the other.
> 
> 

OK, converted sess->session_usage_count to use int instead of atomic_t.

> 
> > +
> > +unsigned char *iscsit_ntoa(u32 ip)
> > +{
> > +	static unsigned char buf[18];
> > +
> > +	memset(buf, 0, 18);
> > +	sprintf(buf, "%u.%u.%u.%u", ((ip>>  24)&  0xff), ((ip>>  16)&  0xff),
> > +			((ip>>  8)&  0xff), (ip&  0xff));
> > +
> > +	return buf;
> > +}
> 
> Nothing is using this. Remove.
> 
> 

Removed

> > +
> > +void iscsit_ntoa2(unsigned char *buf, u32 ip)
> > +{
> > +	memset(buf, 0, 18);
> > +	sprintf(buf, "%u.%u.%u.%u", ((ip>>  24)&  0xff), ((ip>>  16)&  0xff),
> > +			((ip>>  8)&  0xff), (ip&  0xff));
> > +}
> 
> I think we have a function like this already.
> 
>
> If not, I think this should be:
> 
> sprintf(buf, "%pI4",
> 
> What s up with ipv6 btw? That uses %pI6.
> 

Mmm, looking at include/linux/inet.h, I don't see a equivilent for the
iscsit_ntoa2() above AFAICT..

> 
> > +
> > +#define NS_INT16SZ	 2
> > +#define NS_INADDRSZ	 4
> > +#define NS_IN6ADDRSZ	16
> > +
> > +/* const char *
> > + * inet_ntop4(src, dst, size)
> > + *	format an IPv4 address
> > + * return:
> > + *	`dst' (as a const)
> > + * notes:
> > + *	(1) uses no statics
> > + *	(2) takes a unsigned char* not an in_addr as input
> > + * author:
> > + *	Paul Vixie, 1996.
> > + */
> > +static const char *iscsit_ntop4(
> 
> 
> Only used by iscsit_ntop6
> 

Dropped

> 
> > +/* const char *
> > + * isc_inet_ntop6(src, dst, size)
> > + * convert IPv6 binary address into presentation (printable) format
> > + * author:
> > + *	Paul Vixie, 1996.
> > + */
> > +const char *iscsit_ntop6(const unsigned char *src, char *dst, size_t size)
> > +{
> 
> 
> Not used.
> 
> 

Dropped

> > +
> > +/* int
> > + * inet_pton4(src, dst)
> > + *	like inet_aton() but without all the hexadecimal and shorthand.
> > + * return:
> > + *	1 if `src' is a valid dotted quad, else 0.
> > + * notice:
> > + *	does not touch `dst' unless it's returning 1.
> > + * author:
> > + *	Paul Vixie, 1996.
> > + */
> > +static int iscsit_pton4(const char *src, unsigned char *dst)
> > +{
> 
> 
> 
> Only used by inet_pton6
> 

Dropped

> > +
> > +/* int
> > + * inet_pton6(src, dst)
> > + *	convert presentation level address to network order binary form.
> > + * return:
> > + *	1 if `src' is a valid [RFC1884 2.2] address, else 0.
> > + * notice:
> > + *	(1) does not touch `dst' unless it's returning 1.
> > + *	(2) :: in a full address is silently ignored.
> > + * credit:
> > + *	inspired by Mark Andrews.
> > + * author:
> > + *	Paul Vixie, 1996.
> > + */
> > +int iscsit_pton6(const char *src, unsigned char *dst)
> > +{
> > +	static const char xdigits_l[] = "0123456789abcdef",
> 
> 
> 
> Not used.
> 

Dropped.

> I think if you needed these functions then they should go into some 
> network code. There were not specific to a iscsi target were they.
> 
> 

Ok, the only one that I cannot find an externally accessable equivilent
is iscsit_ntoa2(), which I will go ahead and drop..

> 
> > +
> > +	return NULL;
> > +}
> > +
> > +void iscsit_check_conn_usage_count(struct iscsi_conn *conn)
> > +{
> > +	spin_lock_bh(&conn->conn_usage_lock);
> > +	if (atomic_read(&conn->conn_usage_count)) {
> > +		atomic_set(&conn->conn_waiting_on_uc, 1);
> > +		spin_unlock_bh(&conn->conn_usage_lock);
> > +
> > +		wait_for_completion(&conn->conn_waiting_on_uc_comp);
> > +		return;
> > +	}
> > +	spin_unlock_bh(&conn->conn_usage_lock);
> > +}
> > +
> > +void iscsit_dec_conn_usage_count(struct iscsi_conn *conn)
> > +{
> > +	spin_lock_bh(&conn->conn_usage_lock);
> > +	atomic_dec(&conn->conn_usage_count);
> > +
> > +	if (!atomic_read(&conn->conn_usage_count)&&
> > +	     atomic_read(&conn->conn_waiting_on_uc))
> > +		complete(&conn->conn_waiting_on_uc_comp);
> > +
> > +	spin_unlock_bh(&conn->conn_usage_lock);
> > +}
> > +
> > +void iscsit_inc_conn_usage_count(struct iscsi_conn *conn)
> > +{
> > +	spin_lock_bh(&conn->conn_usage_lock);
> > +	atomic_inc(&conn->conn_usage_count);
> > +	spin_unlock_bh(&conn->conn_usage_lock);
> > +}
> 
> Something about atomics and always being accessed under a lock. I think 
> this happens in other places too.
> 
> Could this also be done with krefs?

Converted these as well to int for current code.  Btw, I am happy to
take a patch converting these to krefs.  :)

> 
> > +
> > +int iscsit_check_for_active_network_device(struct iscsi_conn *conn)
> > +{
> > +	struct net_device *net_dev;
> > +
> > +	if (!conn->net_if) {
> > +		printk(KERN_ERR "struct iscsi_conn->net_if is NULL for CID:"
> > +			" %hu\n", conn->cid);
> > +		return 0;
> > +	}
> > +	net_dev = conn->net_if;
> > +
> > +	return netif_carrier_ok(net_dev);
> > +}
> > +
> > +static void iscsit_handle_netif_timeout(unsigned long data)
> > +{
> > +	struct iscsi_conn *conn = (struct iscsi_conn *) data;
> > +
> > +	iscsit_inc_conn_usage_count(conn);
> > +
> > +	spin_lock_bh(&conn->netif_lock);
> > +	if (conn->netif_timer_flags&  ISCSI_TF_STOP) {
> > +		spin_unlock_bh(&conn->netif_lock);
> > +		iscsit_dec_conn_usage_count(conn);
> > +		return;
> > +	}
> > +	conn->netif_timer_flags&= ~ISCSI_TF_RUNNING;
> > +
> > +	if (iscsit_check_for_active_network_device((void *)conn)) {
> > +		iscsit_start_netif_timer(conn);
> > +		spin_unlock_bh(&conn->netif_lock);
> > +		iscsit_dec_conn_usage_count(conn);
> > +		return;
> > +	}
> > +
> > +	printk(KERN_ERR "Detected PHY loss on Network Interface: %s for iSCSI"
> > +		" CID: %hu on SID: %u\n", conn->net_dev, conn->cid,
> > +			conn->sess->sid);
> > +
> > +	spin_unlock_bh(&conn->netif_lock);
> > +
> > +	iscsit_cause_connection_reinstatement(conn, 0);
> > +	iscsit_dec_conn_usage_count(conn);
> > +}
> 
> 
> I think instead of polling the device you can use 
> register_netdevice_notifier. See fcoe.c for an example.
> 
> 

Hmmm, this netif struct net_device timeout code to trigger connection
reinstatement that is disabled in iscsi_target_login.c:
iscsi_post_login_start_timers() because of not having an (easy) mainline
method of locating the underlying struct net_device (or at least AFAICT)
from a kernel-level struct socket.  So with the original code this was
intended to be set by a userspace provided ethX string.

That said, I will go ahead and drop this code for the initial merge.
Once we figure out a proper way to obtain struct net_device from
a struct socket I will re-add this via a register_netdevice_notifier()
callback for the case to trigger an iSCSI connection failure from a
struct net_device state failure for something like a physical link layer
loss.

> 
> > +
> > +static inline int iscsit_do_rx_data(
> > +	struct iscsi_conn *conn,
> > +	struct iscsi_data_count *count)
> > +{
> 
> 
> 
> > +
> > +	while (total_rx<  data) {
> > +		oldfs = get_fs();
> > +		set_fs(get_ds());
> > +
> > +		conn->sock->sk->sk_allocation = GFP_ATOMIC;
> 
> 
> I do not think you need GFP_ATOMIC. If you cannot sleep then I think you 
> are in trouble below, because you pass in MSG_WAITALL.
> 
> I think since this is the target side you can use GFP_KERNEL. Target 
> mode should not have the same allocation swinging around on you 
> dependency problem like a initiator does, does it?
> 
> If it does at least use GFP_NOIO (I think iscsi_tcp.c should be using 
> GFP_NOIO and not GFP_ATOMIC).
> 

Ok, removing sk_allocation = GFP_ATOMIC for both cases..

> And I think we are supposed to be using kernel_recvmsg. It does the 
> get/set_ds stuff for you.
> 
> 

Converted to use kernel_recvmsg() here, and also converted all struct
iovec usage in iscsi_target_mod to struct kvec.

> > +		rx_loop = sock_recvmsg(conn->sock,&msg,
> > +				(data - total_rx), MSG_WAITALL);
> > +
> 
> 
> 
> 
> > +static inline int iscsit_do_tx_data(
> > +	struct iscsi_conn *conn,
> > +	struct iscsi_data_count *count)
> > +{
> 
> 
> 
> > +
> > +	while (total_tx<  data) {
> > +		oldfs = get_fs();
> > +		set_fs(get_ds());
> > +
> > +		conn->sock->sk->sk_allocation = GFP_ATOMIC;
> 
> Same comment as the recv side. I think you can also move this and set it 
> in one place.
> 
> And I think we are supposed to be using kernel_sendmsg. That will also 
> do the get/set_fs stuff for you.
> 
> 

Dropped GFP_ATOMIC and converted to kernel_sendmsg()..

> > +		tx_loop = sock_sendmsg(conn->sock,&msg, (data - total_tx));
> > +
> > +		set_fs(oldfs);
> > +
> 
>
> 
> > +extern int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
> > +{
> > +	char *ip, *payload = NULL;
> > +	struct iscsi_conn *conn = cmd->conn;
> > +	struct iscsi_portal_group *tpg;
> > +	struct iscsi_tiqn *tiqn;
> > +	struct iscsi_tpg_np *tpg_np;
> > +	int buffer_len, end_of_buf = 0, len = 0, payload_len = 0;
> > +	unsigned char buf[256];
> > +	unsigned char buf_ipv4[IPV4_BUF_SIZE];
> > +
> > +	buffer_len = (conn->conn_ops->MaxRecvDataSegmentLength>  32768) ?
> > +			32768 : conn->conn_ops->MaxRecvDataSegmentLength;
> > +
> > +	payload = kzalloc(buffer_len, GFP_KERNEL);
> > +	if (!payload) {
> > +		printk(KERN_ERR "Unable to allocate memory for sendtargets"
> > +			" response.\n");
> > +		return -1;
> > +	}
> > +
> > +	spin_lock(&tiqn_lock);
> > +	list_for_each_entry(tiqn,&g_tiqn_list, tiqn_list) {
> > +		memset(buf, 0, 256);
> > +
> > +		len = sprintf(buf, "TargetName=%s", tiqn->tiqn);
> > +		len += 1;
> > +
> > +		if ((len + payload_len)>  buffer_len) {
> > +			spin_unlock(&tiqn->tiqn_tpg_lock);
> > +			end_of_buf = 1;
> > +			goto eob;
> > +		}
> > +		memcpy((void *)payload + payload_len, buf, len);
> > +		payload_len += len;
> > +
> > +		spin_lock(&tiqn->tiqn_tpg_lock);
> > +		list_for_each_entry(tpg,&tiqn->tiqn_tpg_list, tpg_list) {
> > +
> > +			spin_lock(&tpg->tpg_state_lock);
> > +			if ((tpg->tpg_state == TPG_STATE_FREE) ||
> > +			    (tpg->tpg_state == TPG_STATE_INACTIVE)) {
> > +				spin_unlock(&tpg->tpg_state_lock);
> > +				continue;
> > +			}
> > +			spin_unlock(&tpg->tpg_state_lock);
> > +
> > +			spin_lock(&tpg->tpg_np_lock);
> > +			list_for_each_entry(tpg_np,&tpg->tpg_gnp_list,
> > +					tpg_np_list) {
> > +				memset(buf, 0, 256);
> > +
> > +				if (tpg_np->tpg_np->np_sockaddr.ss_family == AF_INET6) {
> > +					ip =&tpg_np->tpg_np->np_ipv6[0];
> 
> 
> Is ip supposed to be a string with the ip address in it? If so is that 
> right? Is np_ipv6 a string with the ip address in human readable format, 
> but below np_ipv4 is the integer representation then you convert it.
> 
> 

Mmm, I see your point here.  Getting this converted to use a single
np->np_ip[] string, and will do the aton conversition and drop
iscsit_ntoa2() instead.

> > +				} else {
> > +					memset(buf_ipv4, 0, IPV4_BUF_SIZE);
> > +					iscsit_ntoa2(buf_ipv4,
> > +						tpg_np->tpg_np->np_ipv4);
> > +					ip =&buf_ipv4[0];
> > +				}
> > +
> > +				len = sprintf(buf, "TargetAddress="
> > +					"%s%s%s:%hu,%hu",
> > +					(tpg_np->tpg_np->np_sockaddr.ss_family == AF_INET6) ?
> > +					"[" : "", ip,
> > +					(tpg_np->tpg_np->np_sockaddr.ss_family == AF_INET6) ?
> > +					"]" : "", tpg_np->tpg_np->np_port,
> > +					tpg->tpgt);
> > +				len += 1;
> > +
> > +				if ((len + payload_len)>  buffer_len) {
> > +					spin_unlock(&tpg->tpg_np_lock);
> > +					spin_unlock(&tiqn->tiqn_tpg_lock);
> > +					end_of_buf = 1;
> > +					goto eob;
> > +				}
> > +
> > +				memcpy((void *)payload + payload_len, buf, len);
> > +				payload_len += len;
> > +			}
> > +			spin_unlock(&tpg->tpg_np_lock);
> > +		}
> > +		spin_unlock(&tiqn->tiqn_tpg_lock);
> > +eob:
> > +		if (end_of_buf)
> > +			break;
> > +	}
> > +	spin_unlock(&tiqn_lock);
> > +
> > +	cmd->buf_ptr = payload;
> --

I finishing up the last items now, and will create a PATCH-v2 containing
the above changes and seperate patchs for generic iscsi_sna() functions
in iscsi_proto.h, and the conversion of target_core_mod to use
int_to_scsilun() instead of a TFO->pack_lun() vector.

Thank you for your review Mike!

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists