Re: [libvirt] [PATCH 4/5] nodedev: Introduce udevHandleOneDevice
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(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,
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(a)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