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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > > > > Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx>
> > > > > Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > > 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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list