Re: [libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes
On 1/28/21 1:47 PM, Daniel P. Berrangé wrote:
On Thu, Jan 28, 2021 at 01:18:07PM +0100, Michal Privoznik wrote:
> On 1/28/21 11:44 AM, Peter Krempa wrote:
>> On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
>>> If "udevGetDeviceSysfsAttr()" returns NULL,
"udevGetIntSysfsAttr"
>>> would return "0", indicating success, without writing to
"value".
>>>
>>> This was found by clang-tidy's
>>> "clang-analyzer-core.UndefinedBinaryOperatorResult" check in
>>> function "udevProcessCCW", flagging a read on the potentially
>>> uninitialized variable "online".
>>>
>>> Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
>>> ---
>>> src/node_device/node_device_udev.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
>>> index 55a2731681..d5a12bab0e 100644
>>> --- a/src/node_device/node_device_udev.c
>>> +++ b/src/node_device/node_device_udev.c
>>> @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device,
>>> str = udevGetDeviceSysfsAttr(udev_device, attr_name);
>>> - if (str && virStrToLong_i(str, NULL, base, value) < 0) {
>>> + if (!str)
>>> + return -1;
>>
>> In this case an error wouldn't be reported any more.
>
> I think it's quite the opposite actually. Previously, if str == NULL then a
> zero was returned (without any error) from this function. Now you get -1.
>
> I think we want to keep return 0 in case of !str. Callers use the following
> pattern:
>
> var = -1; /* default */
> udevGetIntSysfsAttr(device, "attribute", &var, 10);
>
> If "attribute" exists, @var is updated; if it doesn't it's left
untouched
> with the default value (-1 in this case).
There should not be any code with this pattern because that leads us to
ignore genuine errors.
I was a bit too harsh in my reply. Of course we check for
udevGetIntSysfsAttr() retval. The pattern is like this:
var = -1;
if (udevGetIntSysfsAttr(device, "attribute", &var, 10) < 0)
goto error;
And I think this okay.
If we want to degrade when an attribute isn't present, we must be explict
about that and test existance of the sysfs file
Well, the above example would then look like this:
var = -1;
str = udev_device_get_sysattr_value(device, "attribute");
if (str && virStrToLong_i(str, NULL, &var, 10) < 0) {
/* error */
}
Which is exactly what udevGetIntSysfsAttr() does, except it's open coded.
Michal