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&gt; > > > > --- > > > > 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.