[PATCH 2/4] node_device_udev: Make udevGetStringProperty() return void

[Peter Krempa]

On Wed, Jun 02, 2021 at 09:48:46 +0200, Peter Krempa wrote:
> On Wed, Jun 02, 2021 at 09:37:41 +0200, Michal Privoznik wrote:
> > This function can't fail really as it's returning 0 no matter
> > what. This is probably a residue from old days when we cared
> > about propagating OOM errors. Now we just abort. Make its return
> > type void then.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> > ---
> >  src/node_device/node_device_udev.c | 46 ++++++++++--------------------
> >  1 file changed, 15 insertions(+), 31 deletions(-)
> > 
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index d5f3beb389..f48789d98f 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> 
> [...]
> 
> > @@ -517,10 +515,9 @@ udevProcessUSBDevice(struct udev_device *device,
> >      if (udevGetUintProperty(device, "ID_VENDOR_ID", &usb_dev->vendor, 16) < 0)
> >          return -1;
> >  
> > -    if (udevGetStringProperty(device,
> > -                              "ID_VENDOR_FROM_DATABASE",
> > -                              &usb_dev->vendor_name) < 0)
> > -        return -1;
> > +    udevGetStringProperty(device,
> > +                          "ID_VENDOR_FROM_DATABASE",
> > +                          &usb_dev->vendor_name);
> >  
> >      if (!usb_dev->vendor_name &&
> 
> Here you've kept the NULL check that should be now impossible to trigger
> ...
> 
> >          udevGetStringSysfsAttr(device, "manufacturer",
> > @@ -530,10 +527,9 @@ udevProcessUSBDevice(struct udev_device *device,
> >      if (udevGetUintProperty(device, "ID_MODEL_ID", &usb_dev->product, 16) < 0)
> >          return -1;
> >  
> > -    if (udevGetStringProperty(device,
> > -                              "ID_MODEL_FROM_DATABASE",
> > -                              &usb_dev->product_name) < 0)
> > -        return -1;
> > +    udevGetStringProperty(device,
> > +                          "ID_MODEL_FROM_DATABASE",
> > +                          &usb_dev->product_name);
> >  
> >      if (!usb_dev->product_name &&
> 
> (same here)
> 
> >          udevGetStringSysfsAttr(device, "product",
> 
> [...]
> 
> > @@ -965,8 +954,7 @@ udevProcessStorage(struct udev_device *device,
> >       * expected, so I don't see a problem with not having a property
> >       * for it. */
> >  
> > -    if (udevGetStringProperty(device, "ID_TYPE", &storage->drive_type) < 0)
> > -        goto cleanup;
> > +    udevGetStringProperty(device, "ID_TYPE", &storage->drive_type);
> >  
> >      if (!storage->drive_type ||
> 
> (and here, but with different logic)
> 
> >          STREQ(def->caps->data.storage.drive_type, "generic")) {
> > @@ -1010,9 +998,7 @@ static int
> >  udevProcessSCSIGeneric(struct udev_device *dev,
> >                         virNodeDeviceDef *def)
> >  {
> > -    if (udevGetStringProperty(dev, "DEVNAME", &def->caps->data.sg.path) < 0 ||
> > -        !def->caps->data.sg.path)
> 
> ... but here you've removed it.


Okay, according to the manpage of 'udev_device_get_property_value' [1]
which is called from 'udevGetDeviceProperty' which is in turn called
from 'udevGetStringProperty' 'udev_device_get_property_value' can return
NULL on failure. g_strdup will faithfully duplicate the NULL,
thus this is changing the logic of the return value handling without
justification. Please keep the check in place or justify why it's okay.



[1]

On success, udev_device_get_property_value() and
udev_device_get_sysattr_value() return a pointer to a constant string of
the requested value. On error, NULL is returned. Attributes that may
contain NUL bytes should not be retrieved with
udev_device_get_sysattr_value(); instead, read them directly from the
files within the device's syspath.