Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753312AbbFLDRl (ORCPT ); Thu, 11 Jun 2015 23:17:41 -0400 Received: from st11p01im-asmtp001.me.com ([17.172.204.151]:37427 "EHLO st11p01im-asmtp001.me.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753229AbbFLDRg convert rfc822-to-8bit (ORCPT ); Thu, 11 Jun 2015 23:17:36 -0400 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.14.151,1.0.33,0.0.0000 definitions=2015-06-12_01:2015-06-11,2015-06-12,1970-01-01 signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=7.0.1-1412110000 definitions=main-1506120059 Content-type: text/plain; charset=us-ascii MIME-version: 1.0 (Mac OS X Mail 7.3 \(1878.6\)) Subject: Re: [PATCH] kernel/params.c: make use of unused but set variable From: Louis Langholtz In-reply-to: <20150611015427.GB3216@mtj.duckdns.org> Date: Thu, 11 Jun 2015 21:17:32 -0600 Cc: Linus Torvalds , Linux Kernel Mailing List , trivial@kernel.org, Rusty Russell , Andrew Morton Content-transfer-encoding: 8BIT Message-id: References: <1433721270-9182-1-git-send-email-lou_langholtz@me.com> <20150608000007.GA3543@mtj.duckdns.org> <20150608005848.GA21465@mtj.duckdns.org> <1A6DE1E8-4E0E-4894-A07F-A1EE9AC55835@me.com> <20150611015427.GB3216@mtj.duckdns.org> To: Tejun Heo X-Mailer: Apple Mail (2.1878.6) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7001 Lines: 142 Hi Tejun, On Jun 10, 2015, at 7:54 PM, Tejun Heo wrote: > Hey, Louis. > > On Wed, Jun 10, 2015 at 11:05:21AM -0600, Louis Langholtz wrote: >> The underlying code for sysfs_create_file does call WARN to warn about >> any errors. So it's not like the code is totally silent anyway. Then the unused > > Not any errors. It triggers warning on missing ops and dup file > names. Ah... I see now that the underlying code doesn't warn about all errors. Specifically that code doesn't warn about __kernfs_create_file() returning any error code other than EEXIST. So it's quiet on a few other error codes it can return (like ENOMEM or ENOENT). Warnings are generated when sysfs_create_file() returns EINVAL or EEXIST. > The former is a pretty fundamental usage error and we've had > too many of the latter unhandled despite of __must_check. Admittedly the preconditions for sysfs_create_file() seem obscure to me. Perhaps that's contributed to the problem you've described (some comment documentation for the sysfs_create_file function I think would be useful). And what should be done after this function returns? Having this __must_check says its return value must be used. But how should it be used? More on this question after next quote block... > >> ... >> Having functions marked __must_check seems to make more sense when >> their return values are *always* necessary for calling code to have any >> business calling them. Like when there's one or more future calls to functions >> that have to be made (or not made) for the given resource (the kobject) that >> depend on that function's return value (such that the return value state is >> always a later conditional). That doesn't appear to be the case however for >> sysfs_create_file(). > > I don't know. Sounds like a weird rule. We had __must_check, still > had certain error types unhandled so people added explicit WARN to > force people to look into those issues and that means __must_check > should go away? Taken in light of the sysfs_create_file function the rule seems (at least for the moment) helpful in addressing what should be done about the return value of sysfs_create_file(). Should the caller WARN about all errors returned from sysfs_create_file()? Even those already reported (like when sysfs_create_file() returns EINVAL or EEXIST)? If all errors should be WARNed (by code somewhere) then would it be better to have the code underlying sysfs_create_file() do that? If the code underlying sysfs_create_file() warns about all errors, then no caller needs to duplicate that code. Alternatively the caller could just warn when the return code isn't one that's already be warned about. But as the code is now, that relies on how sysfs_create_file() underlying code is coded to know what's already been warned and breaks as soon as someone changes what sysfs_create_file() warns about without updating what the callers warn about. That solution seems unnecessarily fragile. What else should always be done besides provide WARN for errors? If nothing else should always be done and errors are logged by the underlying code, then what point is left for sysfs_create_file() being marked __must_check? It shouldn't be - at least by this offered rule - since its return value isn't always necessary for the caller to still have business in calling it given that it has a worthwhile side-effect (when it succeeds). > Sure, if it warns on all errors and can return void > then there's nothing to discuss but that's not the case. Here, an > error return indicates userland visible behavior difference. I'd > venture to say that the return value is pretty darn important. I don't mean to diminish the significance of userland visible behavior difference. These are significant. And I don't want sysfs_create_file() to return void. But if there's no clear things that callers of sysfs_create_file() should do based on its return value then marking it __must_check seems wrong. I'm guessing (based on the commit log message) that Andrew originally established the precedent for this marking though to help get rid of cases where callers needed to do something different than they were when errors occurred because their code was blindly assuming success. The return value of kmalloc I'd argue meanwhile should always be used. Oddly it's not marked __must_check. But based on this rule, it makes more sense to. What business would a caller have in calling kmalloc if it's not going to use the return value? There is a side-effect of using up memory but I don't know of a reason that's ever desirable for a properly running kernel if the memory is not used (or at least passed to the free function). Fortunately I don't see any callers that aren't using the return value and marking it __must_check only helps to reaffirm what callers already know. > ... >> Adding __must_check probably made it easier for developers to identify calling >> code that was depending on success from these functions. At the same time, >> it's not true (at least currently) that these return values always need to > > I don't know. The "always" rule you're speaking of seems too > arbitrary and rigid to me. Always is a tricky word and tying oneself > to things like that usually doesn't lead to healthy trade-offs. An understandable concern. I'd not want a rule to be established if it can have holes poked into it. But __must_check is itself an always kind of rule at least in so far as the compiler will always warn on the result being unused (assuming it recognizes the attribute and it hasn't been disabled). So having an always kind of rule for when __must_check should or should not be used seems consistent with what it's being applied to and seems like it'd be great to have if it's solid. > I don't think removing __must_check would be a disaster Sounds like what Rusty suggested too. Though I'm unclear if he preferred removing this __must_check attribute or hacking around it with his suggested doesnt_matter() macro. Hopefully it's the former. > but at the > same time these terminal sysfs functions look like the perfect > candidates for such annotations where error returns indicate subtle > but visible behavior differences visible to userland while not > affecting in-kernel operations at all and thus can be easily ignored. > ... Disagreeing with this seems to put me at odds with Andrew Morton too. Some of the sysfs functions may be proper candidates for this attribute but not sysfs_create_file() as I've argued. Hopefully my not following in these shoes will keep me out of trouble better than my following the BUG_ON shoes (by offering the patch that used BUG_ON) got me into trouble! ;) > Thanks. > > -- > tejun And my thanks to you too. -- Lou-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/