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 [EMAIL PROTECTED] -----
From: [EMAIL PROTECTED]
To: [EMAIL PROTECTED]
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 <[EMAIL PROTECTED]>
To: Christoph Hellwig <[EMAIL PROTECTED]>
Cc: Dan Bar Dov <[EMAIL PROTECTED]>, [EMAIL PROTECTED],
[email protected]
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 -----
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
