Re: [RFC-v3 03/12] iscsi-target: Add TCM v4 compatiable ConfigFScontrol plane
From: Nicholas A. Bellinger
Date: Thu Mar 17 2011 - 20:04:58 EST
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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/