[libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes

Michal Privoznik mprivozn at redhat.com
Thu Jan 28 13:03:25 UTC 2021


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 at 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




More information about the libvir-list mailing list