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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
Although I'm not sure (yet) what value this would provide.
John
>
> cleanup:
> udev_device_unref(device);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
