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

[Martin Kletzander]

On Wed, Dec 21, 2016 at 10:35:47AM +0100, Marc Hartmayer wrote:
>On Thu, Dec 08, 2016 at 01:48 PM +0100, Martin Kletzander <mkletzan at redhat.com> 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 at redhat.com> 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 at linux.vnet.ibm.com>
>>>>>Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>>>>>Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>>>>>---
>>>>> 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.html#udev-new
>>>>>-     *
>>>>>-     * 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?
>

Yeah, in that very particular scenario when the process doesn't get
killed, it is not any more likely for virReportError() to fail than it
is for virReportOOMError() I think.

>
>--
>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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161221/8aa3a238/attachment-0001.sig>