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

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

On Thu, 2011-03-17 at 23:22 +0100, Jesper Juhl wrote:
> On Thu, 17 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.
> > 
> > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > ---
> >  drivers/target/iscsi/iscsi_target_configfs.c   | 1593 ++++++++++++++++++++++++
> >  drivers/target/iscsi/iscsi_target_configfs.h   |    7 +
> >  drivers/target/iscsi/iscsi_target_nodeattrib.c |  264 ++++
> >  drivers/target/iscsi/iscsi_target_nodeattrib.h |   14 +
> >  4 files changed, 1878 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/target/iscsi/iscsi_target_configfs.c
> >  create mode 100644 drivers/target/iscsi/iscsi_target_configfs.h
> >  create mode 100644 drivers/target/iscsi/iscsi_target_nodeattrib.c
> >  create mode 100644 drivers/target/iscsi/iscsi_target_nodeattrib.h
> > 
> > diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> > new file mode 100644
> > index 0000000..ae88092
> > --- /dev/null
> > +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> > @@ -0,0 +1,1593 @@
> > +/*******************************************************************************
> > + * This file contains the configfs implementation for iSCSI Target mode
> > + * from the LIO-Target Project.
> > + *
> > + * ?? Copyright 2007-2011 RisingTide Systems LLC.
> 
> "??" ?
> You have this in other files as well - what's the point of those two 
> question marks?
> 
> 

Mmm, this looks like an email encoding issue.  The actual code is using
the copyright symbol (c) -> © 

> > +struct se_tpg_np *lio_target_call_addnptotpg(
> > +	struct se_portal_group *se_tpg,
> > +	struct config_group *group,
> > +	const char *name)
> > +{
> > +	struct iscsi_portal_group *tpg;
> > +	struct iscsi_tpg_np *tpg_np;
> > +	char *str, *str2, *end_ptr, *ip_str, *port_str;
> > +	struct iscsi_np_addr np_addr;
> > +	u32 ipv4 = 0;
> > +	int ret;
> > +	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);
> > +		return ERR_PTR(-EOVERFLOW);
> > +	}
> > +	memset(buf, 0, MAX_PORTAL_LEN);
> 
> memset(buf, 0, MAX_PORTAL_LEN + 1);
> 
> 

Fixed

> > +#define DEF_NACL_ATTRIB(name)						\
> > +static ssize_t iscsi_nacl_attrib_show_##name(				\
> > +	struct se_node_acl *se_nacl,					\
> > +	char *page)							\
> > +{									\
> > +	struct iscsi_node_acl *nacl = container_of(se_nacl, struct iscsi_node_acl, \
> > +					se_node_acl);			\
> > +	ssize_t rb;							\
> > +									\
> > +	rb = sprintf(page, "%u\n", ISCSI_NODE_ATTRIB(nacl)->name);	\
> > +	return rb;							\
> 
> Why the 'rb' variable? Why not just
> 
> return sprintf(page, "%u\n", ISCSI_NODE_ATTRIB(nacl)->name);
> 
> ?
> 
> 

Fixed

> > +#define __DEF_NACL_AUTH_STR(prefix, name, flags)			\
> > +static ssize_t __iscsi_##prefix##_show_##name(				\
> > +	struct iscsi_node_acl *nacl,					\
> > +	char *page)							\
> > +{									\
> > +	struct iscsi_node_auth *auth = &nacl->node_auth;		\
> > +	ssize_t rb;							\
> > +									\
> > +	if (!capable(CAP_SYS_ADMIN))					\
> > +		return -EPERM;						\
> > +	rb = snprintf(page, PAGE_SIZE, "%s\n", auth->name);		\
> > +	return rb;							\
> 
> Kill 'rb' and just 
> 
>   return snprintf(page, PAGE_SIZE, "%s\n", auth->name);
> 
> I'd say.
> 

Fixed

> 
> > +#define __DEF_NACL_AUTH_INT(prefix, name)				\
> > +static ssize_t __iscsi_##prefix##_show_##name(				\
> > +	struct iscsi_node_acl *nacl,					\
> > +	char *page)							\
> > +{									\
> > +	struct iscsi_node_auth *auth = &nacl->node_auth;		\
> > +	ssize_t rb;							\
> > +									\
> > +	if (!capable(CAP_SYS_ADMIN))					\
> > +		return -EPERM;						\
> > +									\
> > +	rb = snprintf(page, PAGE_SIZE, "%d\n", auth->name);		\
> > +	return rb;							\
> 
> Just kill that pointless 'rb' variable..
> 
> 

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) {
> > +		rb += sprintf(page+rb, "No active iSCSI Session for Initiator"
> > +			" Endpoint: %s\n", se_nacl->initiatorname);
> > +	} else {
> > +		sess = (struct iscsi_session *)se_sess->fabric_sess_ptr;
> > +
> 
> Is this cast needed?
> 
> 

Removed

> > +static ssize_t iscsi_tpg_param_store_##name(				\
> > +	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 *buf;							\
> > +	int ret;							\
> > +									\
> > +	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);				\
> > +	if (!buf)							\
> > +		return -ENOMEM;						\
> > +	snprintf(buf, PAGE_SIZE, "%s=%s", __stringify(name), page);	\
> > +	buf[strlen(buf)-1] = '\0'; /* Kill newline */			\
> > +									\
> > +	if (iscsi_get_tpg(tpg) < 0)					\
> > +		return -EINVAL;						\
> 
> You just leaked "buf".
> 
> 

Fixed.  Thanks for your review Jesper!

--nab