lkml.org 
[lkml]   [2011]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC-v2 03/12] iscsi-target: Add TCM v4 compatiable ConfigFS control plane
    From
    Date
    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



    \
     
     \ /
      Last update: 2011-03-15 05:57    [W:2.861 / U:0.184 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site