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@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.
> >
> [...]
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
