Re: [PATCH 3/3] add open iscsi netlink interface to iscsi transport class

Christoph Hellwig
2005-05-05T11:38:03Z

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