[openib-general] [ANNOUNCE] Initial trunk checkin of ISER initiator

[Grant Grundler]

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