Messages in this thread | | | Date | Tue, 15 Mar 2022 08:57:20 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468 |
| |
Hi Agathe,
On 3/15/22 06:03, Agathe Porte wrote: > Hi Guenter, > > Le 3/15/2022 à 2:22 AM, Guenter Roeck a écrit : >> If of_property_read_string() returns an error, it will not set the pointer >> to &data->channel[channel].label, which by default is NULL because the >> data structure was allocated with devm_kzalloc(). That means tmp464_is_visible() >> will disable the label attribute. I don't see a problem with the current >> code. > > Thanks for the explanation. I agree that there is no problem on this point. > >> There are lots of examples in the kernel where the return value from >> of_property_read_string() is silently ignored. Not a single one of >> those uses a (void) typecast. I don't really want to start making >> such changes just to make static analyzers happy. > > I have to disagree here. Because something has always (not) be done in the past should not be a reason to (not) do it in the future out of pure habit. I did not suggest to add the (void) casts in existing code: I agree it would be a burden with no real added value. > > But making static analyzers happy seems justified *for new code*. It also makes *other developers* more confident, because with the cast we are sure that not checking the return value is very intentional. > > Please enlighten me if there are any downsides that I did not think of and that would block this one-line change. >
Changing the code now would require either a separate patch or a rebase of the hwmon-next tree. Rebasing the hwmon-next tree at this point of the release cycle (a few days before the commit window opens) is something I really don't want to do, leaving the option to add a separate patch for the change. That makes it identical to changing existing code to add the (void).
In addition to that, I do not agree that adding (void) really adds value here; it just says "this is done on purpose" because the static analyzer doesn't know better. 0-day stopped reporting this kind of perceived problem, presumably for good reason. Since the result of the function call is implied in setting or not setting the passed pointer, a return value check or adding (void) is not warranted. This would be different if the property was mandatory, but that is not the case here.
There are lots of other functions in the kernel where return values are not checked, for a variety of reasons. Functions where checking the return value is necessary/mandatory are tagged with __must_check. For others it is left to the caller to decide if a return value should be checked, and if it makes sense / adds value to add (void).
I'll give you another example: cancel_work_sync() and related functions. I am sure your static analyzer will complain about the failure to check its return value in almost all cases. A counter-example is, say, platform_driver_register(), where the return value should really be checked and a (void) typecast should be used if it is not checked on purpose. The problem is that static analyzers can not determine if the return value check is necessary, and should either leave it alone or make reports conditional on some command line option.
Overall we'll have to agree to disagree.
Thanks, Guenter
| |