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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 14 Sep 2010 22:56:49 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Vladislav Bolkhovitin <vst@...b.net>
Cc:	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	scst-devel <scst-devel@...ts.sourceforge.net>,
	James Bottomley <James.Bottomley@...senPartnership.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
	Mike Christie <michaelc@...wisc.edu>,
	Jeff Garzik <jeff@...zik.org>, Vu Pham <vuhuong@...lanox.com>,
	Bart Van Assche <bart.vanassche@...il.com>,
	James Smart <James.Smart@...lex.Com>,
	Joe Eykholt <jeykholt@...co.com>, Andy Yan <ayan@...vell.com>,
	Chetan Loke <generationgnu@...oo.com>,
	Hannes Reinecke <hare@...e.de>,
	Richard Sharpe <realrichardsharpe@...il.com>
Subject: Re: [PATCH 0/17]: New SCSI target framework (SCST) with dev handlers
 and 2 target drivers

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/