Re: [libvirt] [PATCH v3 1/6] nodedev: Introduce udevCheckMonitorFD helper function
[...]
> +
> + if (!udevCheckMonitorFD(udev_monitor, fd))
> + goto unlock;> +
> + if (!(device = udev_monitor_receive_device(udev_monitor))) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("udev_monitor_receive_device returned NULL"));
I almost wonder if it would be better to have this be a
ReportSystemError w/ errno... Not that udev docs give any guidance
there, but maybe it'd help.
Not really, that would imply that we can rely on libudev setting the errno, but
the call can fail in multiple ways the most of which are unrelated to any
system call and given the poor interface libudev has :/ we don't have much of a
choice (but yeah, the error is rather uninformative, but since you have no
control over it, it's the best we have IMO).
> - goto cleanup;
> + goto unlock;
I know this is "existing"; however, if !device, then what's the purpose
of calling udev_device_unref(NULL)? In fact there's one path in
udevGetDMIData which actually checks != NULL before calling - although
it has no reason to since I see no way for it to be NULL at cleanup
(again a different issue).
Also, wouldn't nodeDeviceLock/Unlock wrapping only be necessary within
udevCheckMonitorFD? Why would the udev call need to be wrapped as well?
So, IMHO, again, I'm still not convinced about the whole file descriptor
changing under our hands idea (unrelated - was it actually the correct
wording?). But since the general consensus was to keep the sanity checks, I
moved the @fd extraction within the locks. Now, if we presume such thing can
happen, you don't want anyone touching the driver structure in the meantime,
otherwise you're left with a dangling pointer @udev_monitor and bad things
would happen. This way, you don't have to worry about the driver's structure
getting changed, possibly invalidating the file descriptor (not that we were,
but if we really would/should...).
Erik