Re: [libvirt] [PATCH 4/5] nodedev: Introduce udevHandleOneDevice
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(a)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(a)redhat.com>
Although I'm not sure (yet) what value this would provide.
John
cleanup:
udev_device_unref(device);