[openib-general] [noreply at googlegroups.com: Posting error: open-iscsi]

[Grant Grundler]

Dan,
please don't cross post to closed lists.

It means people on open-iscsi don't see my response
and they can't participate in the conversation.

More open source community email ettiquette at:
	http://www.parisc-linux.org/mailing-lists/index.html

See "Rules" section.


grant

----- Forwarded message from noreply at googlegroups.com -----

From: noreply at googlegroups.com
To: iod00d at hp.com
Subject: Posting error: open-iscsi
X-PMX-Version: 5.0.3.165339, Antispam-Engine: 2.1.0.0, Antispam-Data: 2005.8.18.18
X-Spam-Checker-Version: SpamAssassin 3.0.4 (2005-06-05) on debian.cup.hp.com
X-Spam-Level: 
X-Spam-Status: No, score=-0.5 required=5.0 tests=BAYES_00,NO_REAL_NAME,
	RCVD_BY_IP autolearn=no version=3.0.4

You do not have permission to post to group open-iscsi. You may need to join 
the group before being allowed to post, or this group may not be open to 
posting. 

Visit http://groups.google.com/group/open-iscsi/about to join or learn more about who 
is allowed to post to the group. 

Help on using Google Groups is also available at:
http://groups.google.com/support
From: Grant Grundler <iod00d at hp.com>
To: Christoph Hellwig <hch at lst.de>
Cc: Dan Bar Dov <danb at voltaire.com>, open-iscsi at googlegroups.com,
	openib-general at openib.org
Subject: Re: [openib-general] [ANNOUNCE] Initial trunk checkin of ISER initiator


On Thu, Aug 18, 2005 at 02:36:05PM +0200, Christoph Hellwig wrote:
> > All the iSCSI features including device management are available
> > seamlessly with the iSCSI/ISER initiator. ISER simply puts iSCSI 
> > on steroids.
> > 
> > The ISER implementation makes use of the openIB/kDAPL. Please note 
> > that several kDAPL patches that were submitted to the list are 
> > necessary for this implementation to work.
> 
> The code is complete crap, please remove it again.

Christoph,
While I agree with you, that's not a very constructive approach.
Can you pick 5 things that are brain damaged and point them out?

Here are a few easy ones in iser.h:
* vi: set noautoindent tabstop=4 shiftwidth=4 :

	This doesn't follow kernel coding style.
	Tabstops are *8* spaces.

#ifndef CONFIG_INFINIBAND
#include <dat/kdat.h>
#else
#include <kdapl.h>
...
	WTF? this module won't get built w/o CONFIG_INFINIBAND defined.
	Delete all references to the !CONFIG_INFINIBAND cases.

	Delete the pile of typedef's that are commented out

#ifndef MIN
#define MIN(a,b)    ((a) < (b) ? (a) : (b))
#endif
	delete MIN and MAX. Use min ser_conn_state]


/*! --------------------------------------------------------------------
   [enum iser_conn_state]

   Description: iSER connection state
-------------------------------------------------------------------- */
enum iser_conn_state {

	The comment is utterly useless. Delete it or add some content.
	Ditto for several of the additional enum declaratations further
	down.


/*! --------------------------------------------------------------------
   [struct iser_phys_mem_t]

   Description: Physical address based memory descriptor
-------------------------------------------------------------------- */
struct iser_phys_mem_t {
	uint64_t *addrs;
	int length;
	int offset;
	int data_size;

		Is this a clone of "struct scatterlist"?
		See include/asm-*/scatterlist.h.

//    uint64_t                addr;
//    uint64_t                size;
}; /* iser_phys_mem_t */

	Delete the C++ style comments.

struct iser_op_params_t {
    uint32_t    InitiatorRecvDataSegmentLength; /* 512..2*24-1], default: 8K, func: MIN */
    uint32_t    TargetRecvDataSegmentLength; /* 512..2*24-1], default: 8K, func: MIN */
    uint32_t    FirstBurstLength;   /* [512..2**24-1], default: 64K, func: MIN */
    uint32_t    MaxBurstLength; /* [512..2**24-1], default: 256K, func: MIN */
...

	1) Codingstyle: use tabs, not 4 spaces.
	2) Codingstyle: WankyCapsNames are strongly discouraged.
		(See Chap 4 of linux/Documentation/Codingstyle)

enum iser_op_param_default {
    defaultInitiatorRecvDataSegmentLength = 8*1024,
    defaultTargetRecvDataSegmentLength = 8*1024,
    defaultFirstBurstLength = 64*1024,
...

	Ditto. And why aren't these constants?
	Any advantage to the enum type?
	I'm wondering if any of these is replacable with
	ISER_LOGIN_PHASE_PDU_DATA_LEN or related constants
	defined earlier in the file.


Ok...made it half-way through (line 286 of 649) the first file
and that's only 10% of the .h files in ulp/iser/datamover.
There are 11K lines of .c and 2.5K lines of .h.

I hope that's enough to give Dan Bar Dov an idea of what the problem is.

hth,
grant

----- End forwarded message -----