Re: [ANNOUNCE 1/6] Open-iSCSI High-Performance Initiator for Linux

[Matt Mackall]

On Sun, Mar 06, 2005 at 11:04:51PM -0800, Alex Aizman wrote:
>          SCSI LLDD consists of 3 files:
>         - iscsi_if.c (iSCSI open interface over netlink);
>         - iscsi_tcp.[ch] (iSCSI transport over TCP/IP).
> 
>         Signed-off-by: Alex Aizman <itn780@xxxxxxxxx>
>         Signed-off-by: Dmitry Yusupov <dmitry_yus@xxxxxxxxx>

Some minor comments..

> +int
> +iscsi_control_recv_pdu(iscsi_cnx_h cp_cnx, struct iscsi_hdr *hdr,
> +				char *data, uint32_t data_size)

Return value on the same line as the function name, please.

> +{
> +	struct nlmsghdr	*nlh;
> +	struct sk_buff	*skb;

Tabs except at the beginning of the line are a nuisance. 

> +	if (!skb) {
> +		return -ENOMEM;
> +	}

Drop the braces around single statements.

> +EXPORT_SYMBOL_GPL(iscsi_control_recv_pdu);

Is this more than one module?

> +iscsi_control_cnx_error(iscsi_cnx_h cp_cnx, iscsi_err_e error)

Your type-naming scheme is a little unusual for the kernel. err_e
appears redundant. And _h for handle could simply be _t for (opaque)
type.

> +static void
> +iscsi_if_rx(struct sock *sk, int len)
> +{
> +	struct sk_buff *skb;
> +	while ((skb = skb_dequeue(&sk->sk_receive_queue)) != NULL) {

Assignments in tests are somewhat discouraged and !f is preferred to f
!= NULL.

> +#ifndef DEBUG_ASSERT
> +#ifdef BUG_ON
> +#undef BUG_ON
> +#endif
> +#define BUG_ON(expr)
> +#endif

Don't do that, please. An assertion that's not enabled is worse than
no assertion at all.

> +static inline void
> +iscsi_buf_init_hdr(struct iscsi_conn *conn, struct iscsi_buf *ibuf,
> +		   char *vbuf, u8 *crc)
> +{
> +	iscsi_buf_init_virt(ibuf, vbuf, sizeof(struct iscsi_hdr));
> +	if (conn->hdrdgst_en) {
> +		crypto_digest_init(conn->tx_tfm);
> +		crypto_digest_update(conn->tx_tfm, &ibuf->sg, 1);
> +		crypto_digest_final(conn->tx_tfm, crc);

I believe you'll find that crypto_digest_digest does that all for you.

> +#define iscsi_conn_get(rdd) (struct iscsi_conn*)(rdd)->arg.data
> +#define iscsi_conn_set(rdd, conn) (rdd)->arg.data = conn

Inlines, please. The second one is slightly broken without parens.

> +		 * PDU header scattered accross SKB's,

"across"

> +		switch(conn->in.opcode) {
> +		case ISCSI_OP_SCSI_CMD_RSP:

This switch looks like it could happily be in another function. The
indentation is making it cramped.

> +/*
> + * iscsi_ctask_copy - copy skb bits to the destanation cmd task
> + *
> + * The function calls skb_copy_bits() and updates per-connection and
> + * per-cmd byte counters.
> + */
> +static inline int
> +iscsi_ctask_copy(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask,
> +		void *buf, int buf_size)
> +{

Almost kerneldoc style, but not quite.

> +	    BUG_ON(ctask != (void*)sc->SCp.ptr);

Strange cast here.

> +	    if (sc->use_sg) {
> +		int i;

Mixed tabs and spaces.

> +	default:
> +		BUG_ON(1);

BUG()?

> +static void
> +iscsi_tcp_data_ready(struct sock *sk, int flag)
> +{
> +	struct iscsi_conn *conn = (struct iscsi_conn*)sk->sk_user_data;

Redundant cast here.

> +static void
> +iscsi_write_space(struct sock *sk)
> +{
> +	struct iscsi_conn *conn = (struct iscsi_conn*)sk->sk_user_data;
> +	conn->old_write_space(sk);
> +	debug_tcp("iscsi_write_space: cid %d\n", conn->id);
> +	conn->suspend = 0; wmb();

Sneaky. Separate line, please, and maybe a comment as to why the
barrier is needed.

> +	debug_tcp("sendhdr %lx %d bytes at offset %d sent %d res %d\n",
> +		(long)page_address(buf->sg.page), size, offset, buf->sent, res);

%p for page_address?

> +static inline int
> +iscsi_sendpage(struct iscsi_conn *conn, struct iscsi_buf *buf,
> +	       int *count, int *sent)
> +{
> +	ssize_t (*sendpage)(struct socket *, struct page *, int, size_t, int);
[...]
> +	sendpage = sk->ops->sendpage ? : sock_no_sendpage;
> +
> +	res = sendpage(sk, buf->sg.page, offset, size, flags);

Can't we just do an if (a) a() else b() here? The ? <empty> : syntax
is nonstandard.

> +iscsi_solicit_data_cont(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask,
> +			struct iscsi_r2t_info *r2t, int left)
> +{
> +	struct iscsi_data *hdr;
> +	struct iscsi_data_task *dtask;
> +	struct scsi_cmnd *sc = ctask->sc;
> +	int new_offset;
> +
> +	dtask = mempool_alloc(ctask->datapool, GFP_ATOMIC);
> +	hdr = &dtask->hdr;
> +	hdr->flags = 0;
> +	hdr->rsvd2[0] = hdr->rsvd2[1] = hdr->rsvd3 =
> +		hdr->rsvd4 = hdr->rsvd5 = hdr->rsvd6 = 0;

That looks odd, memset?

> +static void
> +iscsi_unsolicit_data_init(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
> +{
> +	struct iscsi_data *hdr;
> +	struct iscsi_data_task *dtask;
> +
> +	dtask = mempool_alloc(ctask->datapool, GFP_ATOMIC);
> +	hdr = &dtask->hdr;
> +	hdr->rsvd2[0] = hdr->rsvd2[1] = hdr->rsvd3 =
> +		hdr->rsvd4 = hdr->rsvd5 = hdr->rsvd6 = 0;

And that looks familiar..

...

Boy howdy, this patch goes on for ever. Giving up at 9%.

-- 
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/