linux-kernel - Re: [PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client

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:	Wed, 3 Sep 2014 14:29:44 +0530
From:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Benjamin Romer <benjamin.romer@...sys.com>,
	David Kershner <david.kershner@...sys.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org, sparmaintainer@...sys.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of
 do_locked_client_insert

On Wed, Sep 03, 2014 at 11:40:38AM +0300, Dan Carpenter wrote:
> On Tue, Sep 02, 2014 at 11:46:35PM +0530, Sudip Mukherjee wrote:
> > From: Sudip Mukherjee <sudip@...torindia.org>
> 
> I really would prefer if you just figured out your email settings so
> this isn't needed.  The From: header is mostly used for people
> forwarding patches from other people.  We have allowed people to use
> the From header like this if they can't get their corporate email
> configured properly but I try to discorage it.  If everyone starts using
> From headers like this then it becomes a pain to deal with.
> 
I will configure the corporate mail. I am the server admin , so there should
not be any problem in settings. :)

> > 
> > removed unused variables
> > fixed sparse warning of context imbalance in 'do_locked_client_insert'
> >                          different lock contexts for basic block
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@...torindia.org>
> > ---
> > 
> 
> This patch is much better and more interesting, but I still want some
> more changes.
> 
I have already sent v3 of the patch just before your mail , based on 
what greg k-h has suggested about the commnent. Please discard that.

> > v1 of the patch of the patch just fixed the sparse warning.
> > On suggestion of Dan Carpenter v2 is the total rewrite of the function.
> > Two of the function arguments (interruptHandle,channelId) are also not used. Wanted to remove them as well , 
> > but then thought maybe the original author have planned for some use of those variables.
> 
> In the kernel we don't put code in until we are ready to use it.  Don't
> worry about future changes.  But on the other hand, don't remove the
> parameters in this patch because that is doing too many changes in one
> patch.  It would have to be done in a follow on patch if you decide to
> do it.
> 
> > -	if (locked) {
> > -		spin_unlock_irqrestore((spinlock_t *) lock, flags);
> > -		locked = 0;
> > +		goto unlock;
> > +	visor_signalqueue_empty(queueinfo->chan, whichqueue);
> 
> Just remove this function.  But mention it in the changelog in case
> there are side effects.
> 
> > +	/*visor_signal_insert() only return 0 or 1 */
> 
> Don't put obvious comments like this.  A normal reader will assume that
> this function is boolean based on how it is used.
> 
> > +	if (visor_signal_insert(queueinfo->chan, whichqueue, pSignal) == 1) {
> 
> Don't put the == 1.  In terms of English, 1 really is intended as
> "success" and not the number one.  Also don't test for == true or
> == false.
> 
> 	if (foo) {
> 	if (foo == true) {
> 
> These two statement *mean* the same thing in terms of English, but the
> first one is simpler and less wordy.
> 
> regards,
> dan carpenter

thanks
sudip
--
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/