[libvirt] [PATCH] node_device: Check return value for udev_new()
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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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.ht... - * - * indicates no return value other than success, so we don't check - * its return value. - */ udev = udev_new(); + if (!udev) { + virReportOOMError(); + goto cleanup; + } #if HAVE_UDEV_LOGGING /* cast to get rid of missing-format-attribute warning */ udev_set_log_fn(udev, (udevLogFunctionPtr) udevLogFunction); -- 2.5.5
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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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.ht... - * - * 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? Does it really mean just an OOM error? Couldn't we add a proper error message?
#if HAVE_UDEV_LOGGING /* cast to get rid of missing-format-attribute warning */ udev_set_log_fn(udev, (udevLogFunctionPtr) udevLogFunction); -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 30, 2016 at 04:25:58PM +0100, Martin Kletzander 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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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.ht... - * - * 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? Does it really mean just an OOM error? Couldn't we add a proper error message?
RHEL-6 vintage udev can return NULL here too, and it does seem to be just OOM related - other problems are ignored. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Wed, Nov 30, 2016 at 04:25 PM +0100, Martin Kletzander <mkletzan@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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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.ht... - * - * 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(). -- 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
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@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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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.ht... - * - * 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
-- 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
On 12/08/2016 01:48 PM, Martin Kletzander 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@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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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.ht...
- * - * 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 Shouldn't the OOM error method already to be used if only one udev implementation could fail due to not enough memory?
-- 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, Dec 08, 2016 at 01:48 PM +0100, Martin Kletzander <mkletzan@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@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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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.ht... - * - * 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
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@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@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@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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.ht... - * - * 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
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@linux.vnet.ibm.com> --- src/node_device/node_device_udev.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4b81312..693194d 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1491,13 +1491,12 @@ static int nodeStateInitialize(bool privileged, if (udevPCITranslateInit(privileged) < 0) goto cleanup; - /* - * http://www.kernel.org/pub/linux/utils/kernel/hotplug/libudev/libudev-udev.ht... - * - * indicates no return value other than success, so we don't check - * its return value. - */ udev = udev_new(); + if (!udev) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create udev context")); + goto cleanup; + } #if HAVE_UDEV_LOGGING /* cast to get rid of missing-format-attribute warning */ udev_set_log_fn(udev, (udevLogFunctionPtr) udevLogFunction); -- 2.5.5
On Fri, Feb 10, 2017 at 10:44:44AM +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@linux.vnet.ibm.com> --- src/node_device/node_device_udev.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
ACK, sorry for the late review, I added one sentence to the commit message and pushed it. Thanks.
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4b81312..693194d 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1491,13 +1491,12 @@ static int nodeStateInitialize(bool privileged, if (udevPCITranslateInit(privileged) < 0) goto cleanup;
- /* - * http://www.kernel.org/pub/linux/utils/kernel/hotplug/libudev/libudev-udev.ht... - * - * indicates no return value other than success, so we don't check - * its return value. - */ udev = udev_new(); + if (!udev) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create udev context")); + goto cleanup; + } #if HAVE_UDEV_LOGGING /* cast to get rid of missing-format-attribute warning */ udev_set_log_fn(udev, (udevLogFunctionPtr) udevLogFunction); -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Feb 20, 2017 at 02:48 PM +0100, Martin Kletzander <mkletzan@redhat.com> wrote:
On Fri, Feb 10, 2017 at 10:44:44AM +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@linux.vnet.ibm.com> --- src/node_device/node_device_udev.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
ACK, sorry for the late review, I added one sentence to the commit message and pushed it. Thanks.
Thanks!
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4b81312..693194d 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1491,13 +1491,12 @@ static int nodeStateInitialize(bool privileged, if (udevPCITranslateInit(privileged) < 0) goto cleanup;
- /* - * http://www.kernel.org/pub/linux/utils/kernel/hotplug/libudev/libudev-udev.ht... - * - * indicates no return value other than success, so we don't check - * its return value. - */ udev = udev_new(); + if (!udev) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create udev context")); + goto cleanup; + } #if HAVE_UDEV_LOGGING /* cast to get rid of missing-format-attribute warning */ udev_set_log_fn(udev, (udevLogFunctionPtr) udevLogFunction); -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- 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
participants (4)
-
Boris Fiuczynski -
Daniel P. Berrange -
Marc Hartmayer -
Martin Kletzander