Re: [libvirt] [PATCH] node_device: Check return value for udev_new()

On Thu, Dec 08, 2016 at 01:48 PM +0100, Martin Kletzander <mkletzan(a)redhat.com&gt; wrote: > On Thu, Dec 01, 2016 at 01:52:03PM +0100, Marc Hartmayer wrote: >>On Wed, Nov 30, 2016 at 04:25 PM +0100, Martin Kletzander <mkletzan(a)redhat.com&gt; wrote: >>> On Wed, Nov 30, 2016 at 01:45:32PM +0100, Marc Hartmayer wrote: >>>>The comment was actually wrong as >>>>https://www.freedesktop.org/software/systemd/man/udev_new.html >>>>mentions that on failure NULL is returned. >>>> >>>>Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; >>>>Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; >>>>Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; >>>>--- >>>> src/node_device/node_device_udev.c | 10 ++++------ >>>> 1 file changed, 4 insertions(+), 6 deletions(-) >>>> >>>>diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c >>>>index 4b81312..4b0a875 100644 >>>>--- a/src/node_device/node_device_udev.c >>>>+++ b/src/node_device/node_device_udev.c >>>>@@ -1491,13 +1491,11 @@ static int nodeStateInitialize(bool privileged, >>>> if (udevPCITranslateInit(privileged) < 0) >>>> goto cleanup; >>>> >>>>- /* >>>>- * http://www.kernel.org/pub/linux/utils/kernel/hotplug/libudev/libudev-udev... >>>>- * >>>>- * indicates no return value other than success, so we don't check >>>>- * its return value. >>>>- */ >>>> udev = udev_new(); >>>>+ if (!udev) { >>>>+ virReportOOMError(); >>>>+ goto cleanup; >>>>+ } >>> >>> Is that true for other udevs and not just systemd-udev? >> >>I'm not sure about other versions of udev but the NULL pointer is already >>handled in udevStatInitialize() for udev_new() in a likewise manner. >> >>> Does it really mean just an OOM error? >> >>It fails for systemd-udev if malloc/calloc fails => this is most >>likely a OOM error at this point. >> >>> Couldn't we add a proper error message? >> >>In udevStateInitialize() the error handling reports an internal error >>but as the original error is caused by OOM I think we have to use >>virReportOOMError(). >> > > OK, it doesn't make sense for it to return NULL anyway, so I'm OK with > that, but I'd rather use internal error as we're not completely sure all > udev implementations can fail only due to not enough memory. Plus the > internal error will give more information in the logs. > > Martin Polite ping :) And is the internal error still working if we have a OOM?

-- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294