Re: [RFC-v2 03/12] iscsi-target: Add TCM v4 compatiable ConfigFS control plane

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

On Tue, 2011-03-15 at 01:54 +0100, Jesper Juhl wrote:
> On Mon, 14 Mar 2011, Nicholas A. Bellinger wrote:
> 
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch adds support for /sys/kernel/config/target/iscsi using
> > TCM v4.0 compatiable calls following target_core_fabric_configfs.c
> > 
> > This includes a number of iSCSI fabric dependent attributes upon
> > target_core_fabric_configfs.c provided struct config_item_types from
> > include/target/target_core_configfs.hstruct target_fabric_configfs_template
> > 
> > It also includes iscsi_target_nodeattrib.[c,h] for handling the
> > lio_target_nacl_attrib_attrs[] store/show for iSCSI fabric dependent
> > attributes.
> > 
> [...]
> 
> A few minor comments below. Not a thorough review by any stretch of the 
> imagination, just a quick glance and then pointing out a few things that 
> caught my eye.
> 
> 
> > +	op = simple_strtoul(page, &endptr, 0);
> > +	 if ((op != 1) && (op != 0)) {
> 
> Indentation is <tab><space> here, should just be <tab>.
> 
> 

Fixed

> > +		printk(KERN_ERR "Illegal value for tpg_enable: %u\n", op);
> > +		return -EINVAL;
> > +	}
> > +	np = tpg_np->tpg_np;
> > +	if (!(np)) {
> 
> if (!np) {
> 
> 

<nod>, fixed a ton of these earlier this afternoon.

> > +		printk(KERN_ERR "Unable to locate struct iscsi_np from"
> > +				" struct iscsi_tpg_np\n");
> 
> Perhaps put the entire log message text on one line. Makes it easier to 
> grep for.
> 
> 

Mmmmm, yes.  I fixed a bunch of the 80 line warnings at one point for
checkpatch.pl,  but point taken for reference for new printk()'s..


> > +	char buf[MAX_PORTAL_LEN];
> > +
> > +	if (strlen(name) > MAX_PORTAL_LEN) {
> > +		printk(KERN_ERR "strlen(name): %d exceeds MAX_PORTAL_LEN: %d\n",
> > +			(int)strlen(name), MAX_PORTAL_LEN);
> 
> Shouldn't this be 
> 
>      char buf[MAX_PORTAL_LEN + 1];
>      
>      if (strlen(name) > MAX_PORTAL_LEN) {
>              printk(KERN_ERR "strlen(name): %d exceeds MAX_PORTAL_LEN: %d\n",
>                      (int)strlen(name), MAX_PORTAL_LEN);
> 
> so that there is room for the terminating \0 ?
> 

Mmmm, fair point.  Fixed.

> 
> > +	snprintf(buf, MAX_PORTAL_LEN, "%s", name);
> > +
> > +	memset((void *)&np_addr, 0, sizeof(struct iscsi_np_addr));
> 
> Unneeded cast?
> 
> 

Dropped

> > +
> > +	str = strstr(buf, "[");
> > +	if ((str)) {
> 
> if (str) {
> 
> 

Fixed

> > +		str2 = strstr(str, "]");
> > +		if (!(str2)) {
> 
> if (!str2) {
> 
> 

Ditto

> > +			printk(KERN_ERR "Unable to locate trailing \"]\""
> > +				" in IPv6 iSCSI network portal address\n");
> > +			return ERR_PTR(-EINVAL);
> > +		}
> > +		str++; /* Skip over leading "[" */
> > +		*str2 = '\0'; /* Terminate the IPv6 address */
> > +		str2 += 1; /* Skip over the "]" */
> 
> why "str++" vs "str2 += 1" and not "str++" & "str2++" ?
> 

Mmmm, changing to '++' for consistency..

> 
> > +		port_str = strstr(str2, ":");
> > +		if (!(port_str)) {
> 
> if (!port_str) {
> 

Fixed

> > +			printk(KERN_ERR "Unable to locate \":port\""
> > +				" in IPv6 iSCSI network portal address\n");
> > +			return ERR_PTR(-EINVAL);
> > +		}
> > +		*port_str = '\0'; /* Terminate string for IP */
> > +		port_str += 1; /* Skip over ":" */
> 
> port_str++;
> 
> 

Ditto

> > +		np_addr.np_port = simple_strtoul(port_str, &end_ptr, 0);
> > +
> > +		snprintf(np_addr.np_ipv6, IPV6_ADDRESS_SPACE, "%s", str);
> > +		np_addr.np_flags |= NPF_NET_IPV6;
> > +	} else {
> > +		ip_str = &buf[0];
> > +		port_str = strstr(ip_str, ":");
> > +		if (!(port_str)) {
> 
> if (!port_str) {
> 

Fixed

> > +			printk(KERN_ERR "Unable to locate \":port\""
> > +				" in IPv4 iSCSI network portal address\n");
> > +			return ERR_PTR(-EINVAL);
> > +		}
> > +		*port_str = '\0'; /* Terminate string for IP */
> > +		port_str += 1; /* Skip over ":" */
> 
> port_str++;
> 

Fixed

> 
> > +static void lio_target_call_delnpfromtpg(
> > +	struct se_tpg_np *se_tpg_np)
> > +{
> > +	struct iscsi_portal_group *tpg;
> > +	struct iscsi_tpg_np *tpg_np;
> > +	struct se_portal_group *se_tpg;
> > +	int ret = 0;
> > +
> 
> pointless initialization of 'ret' to 0
> 

Removed

> > +	tpg_np = container_of(se_tpg_np, struct iscsi_tpg_np, se_tpg_np);
> > +	tpg = tpg_np->tpg;
> > +	ret = iscsi_get_tpg(tpg);
> 
> 'ret' is assigned to here before it's used.
> 
> 

<nod>

> > +#define DEF_NACL_PARAM(name)						\
> > +static ssize_t iscsi_nacl_param_show_##name(				\
> > +	struct se_node_acl *se_nacl,					\
> > +	char *page)							\
> > +{									\
> > +	struct iscsi_session *sess;						\
> > +	struct se_session *se_sess;						\
> > +	ssize_t rb;							\
> > +									\
> > +	spin_lock_bh(&se_nacl->nacl_sess_lock);				\
> > +	se_sess = se_nacl->nacl_sess;					\
> > +	if (!(se_sess)) {						\
> 
> if (!se_sess) {
> 
> 

Fixed

> > +static ssize_t lio_target_nacl_show_info(
> > +	struct se_node_acl *se_nacl,
> > +	char *page)
> > +{
> > +	struct iscsi_session *sess;
> > +	struct iscsi_conn *conn;
> > +	struct se_session *se_sess;
> > +	unsigned char *ip, buf_ipv4[IPV4_BUF_SIZE];
> > +	ssize_t rb = 0;
> > +
> > +	spin_lock_bh(&se_nacl->nacl_sess_lock);
> > +	se_sess = se_nacl->nacl_sess;
> > +	if (!(se_sess))
> 
> if (!se_sess)
> 
> (not pointing out any more of these)
> 

<nod>, I fixed probably 100 of these this afternoon.  ;)

> 
> > +		rb += sprintf(page+rb, "No active iSCSI Session for Initiator"
> > +			" Endpoint: %s\n", se_nacl->initiatorname);
> > +	else {
> 
> when one branch has {}, please use them for the other as well.
> 
> 

Fixed

> > +			if (conn->net_size == IPV6_ADDRESS_SPACE)
> > +				ip = &conn->ipv6_login_ip[0];
> > +			else {
> 
> if () {
> } else {
> }
> 
> 

Fixed

> > +	u32 cmdsn_depth = 0;
> > +	int ret = 0;
> 
> pointless default init of 'ret' to 0.
> 

Removed

> > +
> > +	cmdsn_depth = simple_strtoul(page, &endptr, 0);
> > +	if (cmdsn_depth > TA_DEFAULT_CMDSN_DEPTH_MAX) {
> > +		printk(KERN_ERR "Passed cmdsn_depth: %u exceeds"
> > +			" TA_DEFAULT_CMDSN_DEPTH_MAX: %u\n", cmdsn_depth,
> > +			TA_DEFAULT_CMDSN_DEPTH_MAX);
> > +		return -EINVAL;
> > +	}
> > +	acl_ci = &se_nacl->acl_group.cg_item;
> > +	if (!(acl_ci)) {
> > +		printk(KERN_ERR "Unable to locatel acl_ci\n");
> > +		return -EINVAL;
> > +	}
> > +	tpg_ci = &acl_ci->ci_parent->ci_group->cg_item;
> > +	if (!(tpg_ci)) {
> > +		printk(KERN_ERR "Unable to locate tpg_ci\n");
> > +		return -EINVAL;
> > +	}
> > +	wwn_ci = &tpg_ci->ci_group->cg_item;
> > +	if (!(wwn_ci)) {
> > +		printk(KERN_ERR "Unable to locate config_item wwn_ci\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (iscsi_get_tpg(tpg) < 0)
> > +		return -EINVAL;
> > +	/*
> > +	 * iscsi_tpg_set_initiator_node_queue_depth() assumes force=1
> > +	 */
> > +	ret = iscsi_tpg_set_initiator_node_queue_depth(tpg,
> > +				config_item_name(acl_ci), cmdsn_depth, 1);
> 
> 'ret' is initialized nicely here before it is used.
> 
> 

<nod>

> 
> > +	ssize_t len = 0;
> > +
> 
> no need to initialize 'len' to 0 here - 
> 

Removed

> > +	spin_lock(&tpg->tpg_state_lock);
> > +	len = sprintf(page, "%d\n",
> > +			(tpg->tpg_state == TPG_STATE_ACTIVE) ? 1 : 0);
> 
>         it's initialized here.
> 
> 

<nod>

> > +static ssize_t lio_target_tpg_store_enable(
> > +	struct se_portal_group *se_tpg,
> > +	const char *page,
> > +	size_t count)
> > +{
> > +	struct iscsi_portal_group *tpg = container_of(se_tpg,
> > +			struct iscsi_portal_group, tpg_se_tpg);
> > +	char *endptr;
> > +	u32 op;
> > +	int ret = 0;
> > +
> 
> pointless init of 'ret'.
> 
> 

Removed.

Thanks alot for your review hereh Jesper.  I will get these changed
pushed into lio-4.1 upstream shortly and included for the follow-up RFC
posting for mainline.

Best Regards,

--nab