1st REVIEW : UNH iSCSI for 2.6-test5

1st REVIEW : UNH iSCSI for 2.6-test5

[jd]

Hi,

Available for first Review & Comments.

The UNH iSCSI Project, a collaborative effort between the
UNH IOL and Hewlett Packard Company in Austin, Texas,
( hosted at http://sourceforge.net/projects/unh-iscsi )
to produce a full feature, generic iSCSI implementation ,
has ported the 2.4.18/20 version of the iSCSI initiator to
Linux 2.6.test5 Kernel for consideration of inclusion for
future distribution.

A patch file is available for review at:

http://www.parisc-linux.org/~jpd/

Files included in the patch :

 drivers/scsi/Makefile
 drivers/scsi/Kconfig
 drivers/scsi/unh_iscsi
 drivers/scsi/unh_iscsi/initiator     # HBA & Data mover
 drivers/scsi/unh_iscsi/common        # Login and shared files
 drivers/scsi/unh_iscsi/security       $ SRP and CHAP login

Please submit comments to :

linux-scsi@vger.kernel.org

Alternatively:

jpd_hp_linux_scsi@yahoo.com

This is an on going development efford in progress,
so recommendations and feedback are greatly appreciated.

Thank you for your time .

John Donnelly
HP Linux Network Development group.





__________________________________
Do you Yahoo!?
Yahoo! SiteBuilder - Free, easy-to-use web site design software
http://sitebuilder.yahoo.com

Re: 1st REVIEW : UNH iSCSI for 2.6-test5

[Jeff Garzik]

jd wrote:
> Hi,
> 
> Available for first Review & Comments.
> 
> The UNH iSCSI Project, a collaborative effort between the
> UNH IOL and Hewlett Packard Company in Austin, Texas,
> ( hosted at http://sourceforge.net/projects/unh-iscsi )
> to produce a full feature, generic iSCSI implementation ,
> has ported the 2.4.18/20 version of the iSCSI initiator to
> Linux 2.6.test5 Kernel for consideration of inclusion for
> future distribution.
> 
> A patch file is available for review at:
> 
> http://www.parisc-linux.org/~jpd/
> 
> Files included in the patch :
> 
>  drivers/scsi/Makefile
>  drivers/scsi/Kconfig
>  drivers/scsi/unh_iscsi
>  drivers/scsi/unh_iscsi/initiator     # HBA & Data mover
>  drivers/scsi/unh_iscsi/common        # Login and shared files
>  drivers/scsi/unh_iscsi/security       $ SRP and CHAP login
> 
> Please submit comments to :
> 
> linux-scsi@vger.kernel.org

Please don't mangle or otherwise use non-RFC email addresses.  When you 
use your normal email address somewhere in the email _anyway_, you're 
annoying valid users while doing nothing to defeat spammers.


comments:

* use generic facility for crc32

* most of the formatting looks good, but some of it looks crazy, like

> +	add_length = scan_table_and_process(sock,
> +										p_param_tbl,
> +										SECURITY_PARAM | INFORMATIONAL_PARAM,
> +										INITIATOR,
> +										inputpdu,
> +										outputpdu,
> +										connection_flags, login_flags);

* indeed, this needs to be legible to people w/ 80-column screens

* I disagree with the need for "my_kmalloc" and "my_kfree".  Ditch the 
allocator and directly use the kernel counterparts.

* function prototypes should use "u32" not "__u32" forms.  Ditto for 
other kernel-internal structures.

* string_llx is not needed.  use sprintf.

* verbosely dumping scsi ops is something that should be more generic 
(or removed)

* authentication should be offloaded to a daemon, as NFSv4 did

* I got tired of scrolling right to read code, at this point

Re: 1st REVIEW : UNH iSCSI for 2.6-test5

[Christoph Hellwig]

Please fix your broken Reply-To, thanks.

On Wed, Sep 24, 2003 at 08:12:28AM -0700, jd wrote:
> 
> Hi,
> 
> Available for first Review & Comments.
> 
> The UNH iSCSI Project, a collaborative effort between the
> UNH IOL and Hewlett Packard Company in Austin, Texas,
> ( hosted at http://sourceforge.net/projects/unh-iscsi )
> to produce a full feature, generic iSCSI implementation ,
> has ported the 2.4.18/20 version of the iSCSI initiator to
> Linux 2.6.test5 Kernel for consideration of inclusion for
> future distribution.

Oh well, yet another iscsi.  So what is in your opinion the
advantage over the cisco implementation, what the disadvantage.
Why don't you two cooperate?  (I'm going to ask the cisco folks
the same soon, no worries).

We're certainly not going to merge two iscsi stacks, just FYI.

I gave the code a quick very quick glance (more later):

 - most files have this license clause:

 The name of IOL and/or UNH may not be used to endorse or promote products
 derived from this software without specific prior written permission.
 
   are you sure this is compatible to the GPL?

 - the intel-licensed files definitly are not GPL-compatible
 - linebreaks after 80 chars.
 - did you read Documentation/CondingStyle?
 - I can't find module_init/init_module.  Where's the entry point
   of this code?
 

Re: 1st REVIEW : UNH iSCSI for 2.6-test5

[Christoph Hellwig]

On Wed, Sep 24, 2003 at 06:39:06PM +0100, Christoph Hellwig wrote:
>  - I can't find module_init/init_module.  Where's the entry point
>    of this code?

Sorry, scrap that - I had a truncated patch here.  But you really
want to move away from oldish scsi_module.c using code to the
modern hotplug model.  See e.g. usb-storage or sbp2 for examples.

Re: 1st REVIEW : UNH iSCSI for 2.6-test5

[Christoph Hellwig]

More comments:

 - unh_iscsi/security should really go away.  We have a nice crypto API
   in 2.6 and late 2.4, but authentification really shouldn't be done
   in kernelspace anyway.
 - kill iscsi_device.c.  A scsi LLDD has no business messing with device
   nodes..
 - you're comment style is strange.  In kernel code we tend to use

/*
 * Foo, blah
 * baz..
 */

not

/*

*/

but that's just a minor nitpick.

Having the module description over the copyright boilerplate also is
very strange.

 - your split into common/ and initiator/ is strange.  You can build
   multiple modules in one directory and that would cleanup your
   includes mess nicely
 - having a single host for all of iscsi looks strange.  Why do you
   do that?
 - the #ifdef K26 is totally unreadble.  Especially when it's around
   code that works for both 2.4 and 2.6..
 - kill my_kmallocd / my_free
 - you seems to not handle lots of error returns, e.g. from down_interruptible
   or kernel_thread
 - the UNH_LOCK/UNH_UNLOCK obsfucation hinders reading
 - kill ISCSI_INITIATOR, there's no need for this #define - just initialize
   the host template directly

Re: 1st REVIEW : UNH iSCSI for 2.6-test5

[jd]

Thank you for feedback. 
I'll post follow ups shortly.

John Donnelly.

////
--- Christoph Hellwig <hch@infradead.org> wrote:
> More comments:
> 
>  - unh_iscsi/security should really go away.  We have a nice crypto
> API
>    in 2.6 and late 2.4, but authentification really shouldn't be done
>    in kernelspace anyway.
>  - kill iscsi_device.c.  A scsi LLDD has no business messing with
> device
>    nodes..
>  - you're comment style is strange.  In kernel code we tend to use
> 
> /*
>  * Foo, blah
>  * baz..
>  */
> 
> not
> 
> /*
> 
> */
> 
> but that's just a minor nitpick.
> 
> Having the module description over the copyright boilerplate also is
> very strange.
> 
>  - your split into common/ and initiator/ is strange.  You can build
>    multiple modules in one directory and that would cleanup your
>    includes mess nicely
>  - having a single host for all of iscsi looks strange.  Why do you
>    do that?
>  - the #ifdef K26 is totally unreadble.  Especially when it's around
>    code that works for both 2.4 and 2.6..
>  - kill my_kmallocd / my_free
>  - you seems to not handle lots of error returns, e.g. from
> down_interruptible
>    or kernel_thread
>  - the UNH_LOCK/UNH_UNLOCK obsfucation hinders reading
>  - kill ISCSI_INITIATOR, there's no need for this #define - just
> initialize
>    the host template directly
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

Re: 1st REVIEW : UNH iSCSI for 2.6-test5

[jd]

--- Jeff Garzik <jgarzik@pobox.com> wrote:
> jd wrote:
> > Hi,
> > 
> > Available for first Review & Comments.
> > 
> > The UNH iSCSI Project, a collaborative effort between the
> > UNH IOL and Hewlett Packard Company in Austin, Texas,
> > ( hosted at http://sourceforge.net/projects/unh-iscsi )
> > to produce a full feature, generic iSCSI implementation ,
> > has ported the 2.4.18/20 version of the iSCSI initiator to
> > Linux 2.6.test5 Kernel for consideration of inclusion for
> > future distribution.
> > 
> > A patch file is available for review at:
> > 
> > http://www.parisc-linux.org/~jpd/
> > 
> > Files included in the patch :
> > 
> >  drivers/scsi/Makefile
> >  drivers/scsi/Kconfig
> >  drivers/scsi/unh_iscsi
> >  drivers/scsi/unh_iscsi/initiator     # HBA & Data mover
> >  drivers/scsi/unh_iscsi/common        # Login and shared files
> >  drivers/scsi/unh_iscsi/security       $ SRP and CHAP login
> > 
> > Please submit comments to :
> > 
> > linux-scsi@vger.kernel.org
> 
> Please don't mangle or otherwise use non-RFC email addresses.  When
> you 
> use your normal email address somewhere in the email _anyway_, you're
> 
> annoying valid users while doing nothing to defeat spammers.
> 
> 
> comments:
> 
> * use generic facility for crc32
> 
> * most of the formatting looks good, but some of it looks crazy, like
> 
> > +	add_length = scan_table_and_process(sock,
> > +										p_param_tbl,
> > +										SECURITY_PARAM | INFORMATIONAL_PARAM,
> > +										INITIATOR,
> > +										inputpdu,
> > +										outputpdu,
> > +										connection_flags, login_flags);
> 
> * indeed, this needs to be legible to people w/ 80-column screens
> 
> * I disagree with the need for "my_kmalloc" and "my_kfree".  Ditch
> the 
> allocator and directly use the kernel counterparts.
> 
> * function prototypes should use "u32" not "__u32" forms.  Ditto for 
> other kernel-internal structures.
> 
> * string_llx is not needed.  use sprintf.
> 
> * verbosely dumping scsi ops is something that should be more generic
> 
> (or removed)
> 
> * authentication should be offloaded to a daemon, as NFSv4 did
> 
> * I got tired of scrolling right to read code, at this point
> 

Thanks for the feedback. 
We're aleady aware of these issues ,
and unfortunately the first couple 
of files in common/* that you got tired
of scrolling through will be eliminated 
eventually anyway. ;) . They happen to 
be shared with other components that are
not in consideration for inclusion.

John Donnelly.



__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

Re: 1st REVIEW : UNH iSCSI for 2.6-test5 - Questions

[jd]

See below ...
--- Christoph Hellwig <hch@infradead.org> wrote:
> More comments:
> 
>  - unh_iscsi/security should really go away.  We have a nice crypto
> API
>    in 2.6 and late 2.4, but authentification really shouldn't be done
>    in kernelspace anyway.

    We're in the process of reviewing that API.

>  - kill iscsi_device.c.  A scsi LLDD has no business messing with
> device
>    nodes..
  DONE .. We never used it anyway .
>  - you're comment style is strange.  In kernel code we tend to use
> 
> /*
>  * Foo, blah
>  * baz..
>  */
> 
> not
> 
> /*
> 
> */
> 

  Multiple authors tend to use different comment styles.
  We've been using more of the standard style recently.
> but that's just a minor nitpick.
> 
> Having the module description over the copyright boilerplate also is
> very strange.

 FIXED.
> 
>  - your split into common/ and initiator/ is strange.  You can build
>    multiple modules in one directory and that would cleanup your
>    includes mess nicely
    Common is shared with another module and commands 
    that is not being included . We're likely keep what is 
    really common and migrate other pieces to an appropriate home.

>  - having a single host for all of iscsi looks strange.  Why do you
>    do that?

 The HBA is one entity for iSCSI. I'm open for suggestions,
 or clarification.
>  - the #ifdef K26 is totally unreadble.  Especially when it's around
>    code that works for both 2.4 and 2.6..
     Well, it was a means to mark the differences in what 
     use to work in 2.4 and required tweeking in 2.6. It also 
     highlighted what really needs more attention. Eventually 
     it will go away. 
>  - kill my_kmallocd / my_free

 We use these as a debug means to find memory links and 
 fix them. It's not intended as a garbage collector if that was
your impression. We will certain make corresponding changes so that it
can be made wrapped in DEBUG defines and use standard malloc/free.
 We would prefer to keep it.
>  - you seems to not handle lots of error returns, e.g. from
> down_interruptible
>    or kernel_thread
    Thanks. We should be more aware of possible errors.
>  - the UNH_LOCK/UNH_UNLOCK obsfucation hinders reading.
     Again. We wrapped the irqsave/restore locks in a define
     for debugging . We would like to keep them, perhaps with 
     a better/more informative name.
>  - kill ISCSI_INITIATOR, there's no need for this #define - just
> initialize
>    the host template directly

   DONE !

Thanks,
John.

> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

Re: 1st REVIEW : UNH iSCSI for 2.6-test5

[jd]

Comments /questions below.
--- Jeff Garzik <jgarzik@pobox.com> wrote:
> jd wrote:
> > Hi,
> > 
> > Available for first Review & Comments.
> > 
> > The UNH iSCSI Project, a collaborative effort between the
> > UNH IOL and Hewlett Packard Company in Austin, Texas,
> > ( hosted at http://sourceforge.net/projects/unh-iscsi )
> > to produce a full feature, generic iSCSI implementation ,
> > has ported the 2.4.18/20 version of the iSCSI initiator to
> > Linux 2.6.test5 Kernel for consideration of inclusion for
> > future distribution.
> > 
> > A patch file is available for review at:
> > 
> > http://www.parisc-linux.org/~jpd/
> > 
> > Files included in the patch :
> > 
> >  drivers/scsi/Makefile
> >  drivers/scsi/Kconfig
> >  drivers/scsi/unh_iscsi
> >  drivers/scsi/unh_iscsi/initiator     # HBA & Data mover
> >  drivers/scsi/unh_iscsi/common        # Login and shared files
> >  drivers/scsi/unh_iscsi/security       $ SRP and CHAP login
> > 
> > Please submit comments to :
> > 
> > linux-scsi@vger.kernel.org
> 
> Please don't mangle or otherwise use non-RFC email addresses.  When
> you 
> use your normal email address somewhere in the email _anyway_, you're
> 
> annoying valid users while doing nothing to defeat spammers.
  
 Yes, there seems to be no way to avoid their trash. It took
 them no time to spam this account after a couple of posts.
> 
> 
> comments:
> 
> * use generic facility for crc32

   We will.
> 
> * most of the formatting looks good, but some of it looks crazy, like
> 
> > +	add_length = scan_table_and_process(sock,
> > +										p_param_tbl,
> > +										SECURITY_PARAM | INFORMATIONAL_PARAM,
> > +										INITIATOR,
> > +										inputpdu,
> > +										outputpdu,
> > +										connection_flags, login_flags);
> 
> * indeed, this needs to be legible to people w/ 80-column screens

    We're working that. Eventually it will be a much cleaner file.
> 
> * I disagree with the need for "my_kmalloc" and "my_kfree".  Ditch
> the 
> allocator and directly use the kernel counterparts.
   We use it for debug to find memory leaks and fix them,
   not as a generally garbage collector. We're likely
   wrap it them in a DEBUG phrase or something better it 
   certainly has helped. 
> 
> * function prototypes should use "u32" not "__u32" forms.  Ditto for 
> other kernel-internal structures.

  I'm not quite sure about this one. I see 
  the majority of scsi code uses int,uint .
  Are you recommending just a more consistent usage ?

  
> 
> * string_llx is not needed.  use sprintf.
   Yes.
> 
> * verbosely dumping scsi ops is something that should be more generic
> 
> (or removed)
> 
> * authentication should be offloaded to a daemon, as NFSv4 did
> 
   It will be .
> * I got tired of scrolling right to read code, at this point
> 


 Thanks.


__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com