Re: [PATCH-next] misc: mic: Use PTR_ERR_OR_ZERO

[Ashutosh Dixit]

On Tue, Jan 23 2018 at 02:55:19 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Jan 23, 2018 at 03:10:09PM -0500, Christopher DÃaz Riveros wrote:
>> Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR
>> 
>> This issue was detected by using the Coccinelle software.
>
> ... and that's a wonderful demonstration of the reasons why PTR_ERR_OR_ZERO()
> is in bad taste.
>
> Look at the callers of that sucker:
>
>         err = scif_anon_inode_getfile(ep);
>         if (err)
>                 goto err_anon_inode;
>
> and
>
>         err = scif_anon_inode_getfile(cep);
>         if (err)
>                 goto scif_accept_error_anon_inode;
>
> See the problem?  What really happens here is that we
> 	* call anon_inode_getfile() and stick the result into ->anon.
> 	* if that was ERR_PTR(), we bugger off
>
> Checking that PTR_ERR_OR_ZERO() is non-zero is a bloody convoluted way of
> spelling
> 	if (IS_ERR(...)) {
> 		err = PTR_ERR(...);
> 		goto sod_off;
> 	}
>
> Your change preserves the ugliness of the original calling conventions;
> the mistake here is making it return an int.  Add the fact that
> all (both) callers are in drivers/misc/mic/scif/scif_api.c and take
> a look at this:
> ; git grep -n -w scif_anon_fops
> drivers/misc/mic/scif/scif_api.c:47:const struct file_operations scif_anon_fops = {
> drivers/misc/mic/scif/scif_epd.h:167:   epd->anon = anon_inode_getfile("scif", &scif_anon_fops, NULL, 0);
> drivers/misc/mic/scif/scif_main.h:213:extern const struct file_operations scif_anon_fops;
> ;
>
> So scif_anon_fops is
> 	* defined in scif_api.c
> 	* declared in scif_main.h
> 	* used only in an inline function defined in scif_epd.h, which is used
> only in scif_api.c
>
> Could you spell "way too convoluted"?
>
> Please, do the following: move that sucker scif_api.c, and make it return
> struct file *.  As in return epd->anon = anon_inode_getfile(...);
> And turn the callers into
> 	if (IS_ERR(whatever_you_call_it(ep))) {
> 		err = PTR_ERR(ep->anon);
> 		goto ...;
> 	}
> Then make scif_anon_fops static and kill that extern in scif_main.h.
>
> <looks at that thing>  What the hell?  <greps>  <swears>  <greps
> for callers of scif_poll()> <swears more>
>
> Let me see if I got it right - *ALL* callers of that thing pass it
> an array consisting of 1 (one) entry.  So playing with poll_wqueues
> and __pollwait() is utterly pointless in scif_poll().  So is grabbing
> references to that struct file, BTW, regardless of everything else.
> And so is having the bloody ->anon at all - __scif_pollfd() should be
> passed NULL as the first argument and that's it.  All it takes is
> a separate queue callback set via init_poll_funcptr() and to hell
> with all those complication.  Oh, and since we don't really need
> dynmic allocations inside that queue callback, table.error crap
> also goes to hell...
>
> Incidentally, since this is one of the two places outside of fs/select.c
> using poll_{init,free}wait(), we can bloody well unexport those - the
> other caller (in drivers/staging/farce^Wcomedi) also uses it for
> single struct file, pinned by the caller.  And while we are at it,
> that would also make struct poll_wqueues private to fs/select.c...
>
> Could Intel folks describe the planned future uses of scif_poll()?
> If it's not going to be used for large sets, the whole thing could
> be simplified quite nicely...

Cleanups and suggestions to improve the code such as those outlined at
the top of this reply are of course welcome. The SCIF API has both user
and kernel space clients, and the kernel API mirrors the user space
API. Similar to user space poll which supports multiple file
descriptors, the kernel client poll API was designed to allow poll on
multiple SCIF endpoints as well. Since the API has been stable for a
couple of years now, our preference is to preserve the API as far as
possible.