From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AH8x227gXIlRBWUaymznVVfSE4EIb4zrtxCFM0z+2K75Soa0eCQkVVGFRQ6xa1QXGiYQEUWn8E53 ARC-Seal: i=1; a=rsa-sha256; t=1516744527; cv=none; d=google.com; s=arc-20160816; b=fCVPp+zqwXXhFUKYe6bY7E1+hkb53nWv0hwjJoQ1UU4+xY18KmJFb2EuHI1yEcZ3bP TTweCgfZD1RkhqBJSj8+cgPIhyTlqIHZGIiZw6x8dic3I1VDB57J8QGyQleRNm2XEJSk hHl+x6q6MlCM69rxBlpO8dHvb6mBQsyl+DYqGs9sRfemvNHcaeWLiRuIlPQynf5Ve5zR mVV1Y8c/2cLQDjFF4+ZFZvDshEqWGoXJsX9NuGXVxlgdHA5jLj6Lnjp7TnIjVjsey2Yf Pg4aeUIfYtza9t3VVjthFi+Gk/0XF4F8c0jSk7M2MOu7yGmfNyiyLHAtHQWd+ud3nJvK Rk7g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:user-agent:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:arc-authentication-results; bh=CbNxrdTv6JFHkv6OehyXT2M5nHa5TH6cS1ztNJZ/CoY=; b=LitJ/i8rvrfA3l3XcKjlXRyvYq6L7yHGoC5QfM8KjirgXFcmojsr69hXob1b4Ke7AB QXDU5jUUBgW7luX0ushR5s3fbqvW5bD2WJFGuBnfjLF3S2D2KlIi6mOwdg70Hnq/cG3j o3v1dY2bkAkT5fXGJk+wO0Oi70qCqLxmOno9D+gCROM6W1sim0IAyK+/eU0Euy4wjQZm yLg0FyvoYOzpDJOJXhC6FRZlQXYYGpvNS6MNBNOxQxpEgDB11nSoD3bSndhWbcKFVGbU FR39zp1gfxPL0RElfaf6yN7aX/Pfvkvq9kO7ZoehK0D6fnaOf4Rr7RF0agYrh0ib6sIq TnHA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of viro@ftp.linux.org.uk designates 195.92.253.2 as permitted sender) smtp.mailfrom=viro@ftp.linux.org.uk Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of viro@ftp.linux.org.uk designates 195.92.253.2 as permitted sender) smtp.mailfrom=viro@ftp.linux.org.uk Date: Tue, 23 Jan 2018 21:55:19 +0000 From: Al Viro To: Christopher =?iso-8859-1?Q?D=EDaz?= Riveros Cc: sudeep.dutt@intel.com, ashutosh.dixit@intel.com, arnd@arndb.de, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: Re: [PATCH-next] misc: mic: Use PTR_ERR_OR_ZERO Message-ID: <20180123215519.GP13338@ZenIV.linux.org.uk> References: <20180123201009.12998-1-chrisadr@gentoo.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180123201009.12998-1-chrisadr@gentoo.org> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: Al Viro X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1590415328315757561?= X-GMAIL-MSGID: =?utf-8?q?1590421907912586286?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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. What the hell? 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...