From: Christoph Hellwig <hch@infradead.org>
To: Mike Christie <michaelc@cs.wisc.edu>
Cc: linux-kernel@vger.kernel.org,
linux-scsi <linux-scsi@vger.kernel.org>,
netdev <netdev@oss.sgi.com>
Subject: Re: [PATCH 3/3] add open iscsi netlink interface to iscsi transport class
Date: Thu, 5 May 2005 12:37:34 +0100 [thread overview]
Message-ID: <20050505113734.GA8985@infradead.org> (raw)
In-Reply-To: <42798ADD.5070803@cs.wisc.edu>
[Mike, can you please insert the patches inline? I hate needing to save
attachment first to be able to comment on the patch]
- * Copyright (C) Mike Christie, 2004
+ * Copyright (C) Mike Christie, Dmitry Yusupov, Alex Aizman, 2004 - 2005
should probably be separate Copyright lines for everyone involved.
+static struct iscsi_transport *transport_table[ISCSI_TRANSPORT_MAX];
please avoid static limits for the number of transports, e.g. use the
lib/idr.c helpers.
+static DECLARE_MUTEX(callsema);
horrible name for a lock, even a static one.
+
+struct mempool_zone {
+ mempool_t *pool;
+ volatile int allocated;
don't use volatile, either atomic_t or if it's properly locked just int
+
+#define Z_SIZE_REPLY NLMSG_SPACE(sizeof(struct iscsi_uevent))
+#define Z_MAX_REPLY 8
+#define Z_HIWAT_REPLY 6
+
+#define Z_SIZE_PDU NLMSG_SPACE(sizeof(struct iscsi_uevent) + \
+ sizeof(struct iscsi_hdr) + \
+ DEFAULT_MAX_RECV_DATA_SEGMENT_LENGTH)
+#define Z_MAX_PDU 8
+#define Z_HIWAT_PDU 6
+
+#define Z_SIZE_ERROR NLMSG_SPACE(sizeof(struct iscsi_uevent))
+#define Z_MAX_ERROR 16
+#define Z_HIWAT_ERROR 12
At least the *_Z_SIZE defines are unneeded, just pass them to the functions
directly. And please add some explanations for the MAX and HIWAT defines.
+struct iscsi_if_cnx {
+ struct list_head item; /* item in cnxlist */
+ struct list_head snxitem; /* item in snx->connections */
please rename cnx to conn and snx to session everywhere, keeps the code
a lot more readable.
+ iscsi_cnx_t cnxh;
+ volatile int active;
volatile usage again
+#define H_TYPE_TRANS 1
+#define H_TYPE_HOST 2
+static struct iscsi_if_cnx*
+iscsi_if_find_cnx(uint64_t key, int type)
+{
+ unsigned long flags;
+ struct iscsi_if_cnx *cnx;
+
+ spin_lock_irqsave(&cnxlock, flags);
+ list_for_each_entry(cnx, &cnxlist, item) {
+ if ((type == H_TYPE_TRANS && cnx->cnxh == key) ||
+ (type == H_TYPE_HOST && cnx->host == iscsi_ptr(key))) {
+ spin_unlock_irqrestore(&cnxlock, flags);
+ return cnx;
+ }
+ }
+ spin_unlock_irqrestore(&cnxlock, flags);
+ return NULL;
}
please use two separate helpers for transport/host instead of the H_TYPE
thing.
+ list_del((void*)&skb->cb);
please add some inline helpers for using skb->cb as list instead of
spreading the casts all over.
+static int zone_init(struct mempool_zone *zp, unsigned max,
+ unsigned size, unsigned hiwat)
should probably become mempool_zone_init to match the other functions
operating on struct mempool_zone.
+static int
+iscsi_if_destroy_snx(struct iscsi_transport *transport, struct iscsi_uevent *ev)
+{
+ struct Scsi_Host *shost;
+ struct iscsi_if_snx *snx;
+ unsigned long flags;
+ struct iscsi_if_cnx *cnx;
+
+ shost = scsi_host_lookup(ev->u.d_session.sid);
+ if (shost == ERR_PTR(-ENXIO))
+ return -EEXIST;
+ scsi_host_put(shost);
you must keep the reference until you're done.
+ spin_unlock_irqrestore(&cnxlock, flags);
+
+ scsi_remove_host(shost);
+ transport->destroy_session(ev->u.d_session.session_handle);
can we please move the scsi_remove_host into the individual ->destroy_session
methods? dito for the scsi_host_add
+static int
+iscsi_if_create_cnx(struct iscsi_transport *transport, struct iscsi_uevent *ev)
+{
+ struct iscsi_if_snx *snx;
+ struct Scsi_Host *shost;
+ struct iscsi_if_cnx *cnx;
+ unsigned long flags;
+ int error;
+
+ shost = scsi_host_lookup(ev->u.c_cnx.sid);
+ if (shost == ERR_PTR(-ENXIO))
+ return -EEXIST;
+ scsi_host_put(shost);
again, please keep the reference until you're done.
next prev parent reply other threads:[~2005-05-05 11:38 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2005-05-05 2:54 [PATCH 3/3] add open iscsi netlink interface to iscsi transport class Mike Christie
2005-05-05 5:38 ` Chris Wright
2005-05-05 11:37 ` Christoph Hellwig [this message]
2005-05-15 19:30 Mike Christie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20050505113734.GA8985@infradead.org \
--to=hch@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=michaelc@cs.wisc.edu \
--cc=netdev@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).