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

[Daniel P. Berrangé]

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.

If we want to degrade when an attribute isn't present, we must be explict
about that and test existance of the sysfs file


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|