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 <[EMAIL PROTECTED]>
> Signed-off-by: Dmitry Yusupov <[EMAIL PROTECTED]>
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-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
