linux-kernel - Re: [PATCH v7] staging: unisys: visorhba: Convert module from IDR to XArray

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 04 May 2021 15:58:02 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     outreachy-kernel@...glegroups.com,
        David Kershner <david.kershner@...sys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        sparmaintainer@...sys.com, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org, Daniel Vetter <daniel@...ll.ch>,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH v7] staging: unisys: visorhba: Convert module from IDR to XArray

On Tuesday, May 4, 2021 3:42:16 PM CEST Matthew Wilcox wrote:
> On Tue, May 04, 2021 at 03:32:53PM +0200, Fabio M. De Francesco wrote:
> > Changes from v6; Added a call to xa_destroy() that I had forgotten.
> 
> What?  No!  Go back and re-read what I wrote about this previously.
>
I remember that explanation you gave me some days ago for not using it. But I 
was mislead by a comment ("Do we not have to call xa_destroy()?") by Dan and 
your "Correct" soon after the above comment. So I thought that I had 
misunderstand and the put back that call to xa_destroy(). I lost something in 
following the flow of the reviews, I suppose.
> 
> > +static int setup_scsitaskmgmt_handles(struct xarray *xa, struct uiscmdrsp 
*cmdrsp,
> > 
> >  				       wait_queue_head_t *event, 
int *result)
> >  
> >  {
> > 
> > -	/* specify the event that has to be triggered when this */
> > -	/* cmd is complete */
> > -	cmdrsp->scsitaskmgmt.notify_handle =
> > -		simple_idr_get(idrtable, event, lock);
> > -	cmdrsp->scsitaskmgmt.notifyresult_handle =
> > -		simple_idr_get(idrtable, result, lock);
> > +	int ret;
> > +	u32 id;
> > +
> > +	/* specify the event that has to be triggered when this cmd is 
complete */
> > +	ret = xa_alloc_irq(xa, &id, event, xa_limit_32b, GFP_KERNEL);
> > +	if (ret)
> > +		return ret;
> > +	else
> > +		cmdrsp->scsitaskmgmt.notify_handle = id;
> 
> This 'else' is actively confusing.
>
Unnecessary and redundant, yes.
> 
> > +	ret = xa_alloc_irq(xa, &id, result, xa_limit_32b, GFP_KERNEL);
> > +	if (ret) {
> > +		xa_erase_irq(xa, cmdrsp->scsitaskmgmt.notify_handle);
> > +		return ret;
> > +	} else
> > +		cmdrsp->scsitaskmgmt.notifyresult_handle = id;
> 
> Ditto.
>
Redundant, again.

Thanks,

Fabio