[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.
|