Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR
This issue was detected by using the Coccinelle software.
Signed-off-by: Christopher Díaz Riveros <[email protected]>
---
drivers/misc/mic/scif/scif_epd.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/misc/mic/scif/scif_epd.h b/drivers/misc/mic/scif/scif_epd.h
index f39b663da287..b2a835665390 100644
--- a/drivers/misc/mic/scif/scif_epd.h
+++ b/drivers/misc/mic/scif/scif_epd.h
@@ -165,9 +165,7 @@ static inline int scif_verify_epd(struct scif_endpt *ep)
static inline int scif_anon_inode_getfile(scif_epd_t epd)
{
epd->anon = anon_inode_getfile("scif", &scif_anon_fops, NULL, 0);
- if (IS_ERR(epd->anon))
- return PTR_ERR(epd->anon);
- return 0;
+ return PTR_ERR_OR_ZERO(epd->anon);
}
static inline void scif_anon_inode_fput(scif_epd_t epd)
--
2.16.0
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...
Am 23.01.2018 21:10, schrieb Christopher Díaz Riveros:
> Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Christopher Díaz Riveros <[email protected]>
> ---
> drivers/misc/mic/scif/scif_epd.h | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/misc/mic/scif/scif_epd.h b/drivers/misc/mic/scif/scif_epd.h
> index f39b663da287..b2a835665390 100644
> --- a/drivers/misc/mic/scif/scif_epd.h
> +++ b/drivers/misc/mic/scif/scif_epd.h
> @@ -165,9 +165,7 @@ static inline int scif_verify_epd(struct scif_endpt *ep)
> static inline int scif_anon_inode_getfile(scif_epd_t epd)
> {
> epd->anon = anon_inode_getfile("scif", &scif_anon_fops, NULL, 0);
> - if (IS_ERR(epd->anon))
> - return PTR_ERR(epd->anon);
> - return 0;
> + return PTR_ERR_OR_ZERO(epd->anon);
> }
>
the patch looks ok,
but someone should thing about if it makes sense to have a oneliner as function.
IMHO this will only confuse readers (note: yes, there are reasons in some cases,
but i do not see what applies here if any).
re,
wh
> static inline void scif_anon_inode_fput(scif_epd_t epd)
On Tue, Jan 23 2018 at 02:55:19 PM, Al Viro <[email protected]> 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.