Re: [PATCH 0/17]: New SCSI target framework (SCST) with dev handlers and 2 target drivers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Vladislav,

On Tue, Sep 14, 2010 at 06:36:30PM +0400, Vladislav Bolkhovitin wrote:
> Hi All,
> 
> Please review this second iteration of the patch set of the new
> (although, perhaps, the oldest) SCSI target framework for Linux SCST
> with a set of dev handlers and 2 target drivers: for local access
> (scst_local) and for Infiniband SRP (srpt).
> 

I am afraid that the way the code was split into the patches will hinder
the review process. Normally every patch consists of a usable on its
own piece of code (or a logically compete change). This way the unit of
work (coding, testing or reviewing) is clearly defined and easier to
comprehend.

Your patch series, unfortunately, splits Makefile and Kconfig changes
separately from the drivers (which will cause build breakages should it
be applied as is and someone needs to bisect), introduces header files
in their entirety even when some of the data there is not needed till
much later, uses facilities (for example sysfs bindings) that have not
been introduced yet... I am afraid you'd have to rework the splitting
process.

Also patch descriptions should be improved:

"This patch contains SYSFS interface implementation"

- what are you trying to solve?
- main points?
- why offloading of actions to a separate thread?

This should be in the patch description.

BTW, why are you using an exclusive thread instead of exclusive
workqueue for this?

Also, kobject_del() + kobject_put() == kobject_unregister().

And too many "naked returns", I believe common style is to only have
return for non-void functions.

Thanks.

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


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux