[libvirt] [PATCH 4/5] nodedev: Introduce udevHandleOneDevice

[John Ferlan]

On 07/26/2017 02:44 AM, Erik Skultety wrote:
> Let this new method handle the device object we obtained from the
> monitor in order to enhance readability.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/node_device/node_device_udev.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index cd19e79c1..7ecb330df 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1599,6 +1599,23 @@ nodeStateCleanup(void)
>  }
>  
>  
> +static int

Caller never checks status, so what's the point?  I'd just go with void,
unless at some point in time you were going to provide some sort of
error/warning that the action failed or was unrecognized, similar to
when the udevAddOneDevice call in udevProcessDeviceListEntry fails...

Perhaps "the next" step for this function could be:

if ((STREQ("add") || STREQ("change)) &&
    udevAdd() == 0)
    return;

if (STREQ("remove") && udevRemove() == 0)
    return;

VIR_WARN(), using something like:

"Failed to %s device %s", NULLSTR(action),
NULLSTR(udev_device_get_syspath(device));

I use NULLSTR only because on failure it could return NULL... Similarly,
action could be NULL according to the man page... Of course that means
those STREQ's should be STREQ_NULLABLE.

I'd use VIR_WARN unless you're really handling the failure somehow... At
least VIR_WARN will hopefully write something somewhere.


> +udevHandleOneDevice(struct udev_device *device)
> +{
> +    const char *action = udev_device_get_action(device);
> +
> +    VIR_DEBUG("udev action: '%s'", action);
> +
> +    if (STREQ(action, "add") || STREQ(action, "change"))
> +        return udevAddOneDevice(device);
> +
> +    if (STREQ(action, "remove"))
> +        return udevRemoveOneDevice(device);
> +
> +    return 0;
> +}
> +
> +
>  static void
>  udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>                          int fd,
> @@ -1607,7 +1624,6 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>  {
>      struct udev_device *device = NULL;
>      struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> -    const char *action = NULL;
>      int udev_fd = -1;
>  
>      udev_fd = udev_monitor_get_fd(udev_monitor);
> @@ -1635,18 +1651,7 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>          goto cleanup;
>      }
>  
> -    action = udev_device_get_action(device);
> -    VIR_DEBUG("udev action: '%s'", action);
> -
> -    if (STREQ(action, "add") || STREQ(action, "change")) {
> -        udevAddOneDevice(device);
> -        goto cleanup;
> -    }
> -
> -    if (STREQ(action, "remove")) {
> -        udevRemoveOneDevice(device);
> -        goto cleanup;
> -    }
> +    udevHandleOneDevice(device);

Not everyone likes the ignore_value();, so since we're not deciding to
bail if the handling fails, I think using void for the function is fine.

Still for pure code motion... You have my:

Reviewed-by: John Ferlan <jferlan at redhat.com>

Although I'm not sure (yet) what value this would provide.

John

>  
>   cleanup:
>      udev_device_unref(device);
>