| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <alpine.LNX.2.00.1103150130390.26293@swampdragon.chaosbits.net>
Date: Tue, 15 Mar 2011 01:54:04 +0100 (CET)
From: Jesper Juhl <jj@...osbits.net>
To: "Nicholas A. Bellinger" <nab@...ux-iscsi.org>
cc: linux-scsi <linux-scsi@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
James Bottomley <James.Bottomley@...senPartnership.com>,
Christoph Hellwig <hch@....de>,
Mike Christie <michaelc@...wisc.edu>,
Hannes Reinecke <hare@...e.de>,
FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
Boaz Harrosh <bharrosh@...asas.com>,
Stephen Rothwell <sfr@...b.auug.org.au>,
Douglas Gilbert <dgilbert@...erlog.com>
Subject: Re: [RFC-v2 03/12] iscsi-target: Add TCM v4 compatiable ConfigFS
control plane
On Mon, 14 Mar 2011, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@...ux-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>.
> + printk(KERN_ERR "Illegal value for tpg_enable: %u\n", op);
> + return -EINVAL;
> + }
> + np = tpg_np->tpg_np;
> + if (!(np)) {
if (!np) {
> + 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.
> + 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 ?
> + snprintf(buf, MAX_PORTAL_LEN, "%s", name);
> +
> + memset((void *)&np_addr, 0, sizeof(struct iscsi_np_addr));
Unneeded cast?
> +
> + str = strstr(buf, "[");
> + if ((str)) {
if (str) {
> + str2 = strstr(str, "]");
> + if (!(str2)) {
if (!str2) {
> + 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++" ?
> + port_str = strstr(str2, ":");
> + if (!(port_str)) {
if (!port_str) {
> + 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++;
> + 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) {
> + 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++;
> +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
> + 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.
> +#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) {
> +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)
> + 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.
> + if (conn->net_size == IPV6_ADDRESS_SPACE)
> + ip = &conn->ipv6_login_ip[0];
> + else {
if () {
} else {
}
> + u32 cmdsn_depth = 0;
> + int ret = 0;
pointless default init of 'ret' to 0.
> +
> + 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.
> + ssize_t len = 0;
> +
no need to initialize 'len' to 0 here -
> + spin_lock(&tpg->tpg_state_lock);
> + len = sprintf(page, "%d\n",
> + (tpg->tpg_state == TPG_STATE_ACTIVE) ? 1 : 0);
it's initialized here.
> +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'.
--
Jesper Juhl <jj@...osbits.net> http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists