Let this new method handle the device object we obtained from the
monitor in order to enhance readability.

Signed-off-by: Erik Skultety <eskultet@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
+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);
 
  cleanup:
     udev_device_unref(device);
-- 
2.13.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@redhat.com>

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

John

>  
>   cleanup:
>      udev_device_unref(device);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Aug 01, 2017 at 03:46:44PM -0400, John Ferlan wrote:
>
>
> 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@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,

I didn't consider other approaches that's all, I used it to be able directly
return "a call" to another function without thinking about it much - void works
nicely as well.

> 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:

warning is the default log level and there are a number of devices withing the
system which we don't care about and if debug is enabled you'd see messages
like "Discarding device...", so by using warnings here, you'd just pollute the
logs the same way, not sure whether that's desirable, since 1) you can't tell
whether we discarded the device on purpose (which is the case most of the time)
or 2) there's a real problem with the 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@redhat.com>
>
> Although I'm not sure (yet) what value this would provide.

I added a handle thread in a follow-up series where the thread routine invokes
this function and I wanted to make the thread routine a few lines shorter and
"cleaner", no additional value added.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list