Re: [libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes
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).
Michal