Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752300AbbFKByn (ORCPT ); Wed, 10 Jun 2015 21:54:43 -0400 Received: from mail-pd0-f196.google.com ([209.85.192.196]:32777 "EHLO mail-pd0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751779AbbFKByf (ORCPT ); Wed, 10 Jun 2015 21:54:35 -0400 Date: Thu, 11 Jun 2015 10:54:27 +0900 From: Tejun Heo To: Louis Langholtz Cc: Linus Torvalds , Linux Kernel Mailing List , trivial@kernel.org, Rusty Russell , Andrew Morton Subject: Re: [PATCH] kernel/params.c: make use of unused but set variable Message-ID: <20150611015427.GB3216@mtj.duckdns.org> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1A6DE1E8-4E0E-4894-A07F-A1EE9AC55835@me.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2800 Lines: 56 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. The former is a pretty fundamental usage error and we've had too many of the latter unhandled despite of __must_check. > but set variable in params.c (that's set to its return value) can be removed > (and the compiler warning resolved). Incidentally, I do think it'd be helpful > to comment document this behavior of using WARN so that callers can > more readily recognize (and be assured that) they won't need to call WARN > (unless callers want to add __FILE__ and __LINE__ info to the printk output). > > 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? 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. ... > 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. I don't think removing __must_check would be a disaster 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. Thanks. -- tejun -- 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/