Re: [PATCH 1/9] hw/usb/redirect.c: Stop using qemu_oom_check()

[Peter Maydell]

On Sat, 26 Feb 2022 at 18:07, Peter Maydell <peter.maydell@linaro.org> wrote:

Forgot to cc Gerd on this one as USB maintainer. Sorry..

> qemu_oom_check() is a function which essentially says "if you pass me
> a NULL pointer then print a message then abort()".  On POSIX systems
> the message includes strerror(errno); on Windows it includes the
> GetLastError() error value printed as an integer.
>
> Other than in the implementation of qemu_memalign(), we use this
> function only in hw/usb/redirect.c, for three checks:
>
>  * on a call to usbredirparser_create()
>  * on a call to usberedirparser_serialize()
>  * on a call to malloc()
>
> The usbredir library API functions make no guarantees that they will
> set errno on errors, let alone that they might set the
> Windows-specific GetLastError string.  malloc() is documented as
> setting errno, not GetLastError -- and in any case the only thing it
> might set errno to is ENOMEM.  So qemu_oom_check() isn't the right
> thing for any of these.  Replace them with straightforward
> error-checking code.  This will allow us to get rid of
> qemu_oom_check().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I have left all of these errors as fatal, since that's what they
> were previously. Possibly somebody with a better understanding
> of the usbredir code might be able to make them theoretically
> non-fatal, but we make malloc failures generally fatal anyway.
> ---
>  hw/usb/redirect.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 5f0ef9cb3b0..8692ea25610 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1239,7 +1239,11 @@ static void usbredir_create_parser(USBRedirDevice *dev)
>
>      DPRINTF("creating usbredirparser\n");
>
> -    dev->parser = qemu_oom_check(usbredirparser_create());
> +    dev->parser = usbredirparser_create();
> +    if (!dev->parser) {
> +        error_report("usbredirparser_create() failed");
> +        exit(1);
> +    }
>      dev->parser->priv = dev;
>      dev->parser->log_func = usbredir_log;
>      dev->parser->read_func = usbredir_read;
> @@ -2239,7 +2243,10 @@ static int usbredir_put_parser(QEMUFile *f, void 
> *priv, size_t unused,
>      }
>
>      usbredirparser_serialize(dev->parser, &data, &len);
> -    qemu_oom_check(data);
> +    if (!data) {
> +        error_report("usbredirparser_serialize failed");
> +        exit(1);
> +    }
>
>      qemu_put_be32(f, len);
>      qemu_put_buffer(f, data, len);
> @@ -2330,7 +2337,11 @@ static int usbredir_get_bufpq(QEMUFile *f, void *priv, 
> size_t unused,
>          bufp->len = qemu_get_be32(f);
>          bufp->status = qemu_get_be32(f);
>          bufp->offset = 0;
> -        bufp->data = qemu_oom_check(malloc(bufp->len)); /* regular malloc! */
> +        bufp->data = malloc(bufp->len); /* regular malloc! */
> +        if (!bufp->data) {
> +            error_report("usbredir_get_bufpq: out of memory");
> +            exit(1);
> +        }
>          bufp->free_on_destroy = bufp->data;
>          qemu_get_buffer(f, bufp->data, bufp->len);
>          QTAILQ_INSERT_TAIL(&endp->bufpq, bufp, next);
> --
> 2.25.1