| Message ID | 20220226180723.1706285-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Cleanup of qemu_oom_check() and qemu_memalign() | expand |
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
On 2/26/22 08:07, Peter Maydell wrote: > 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(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 26/2/22 19:07, Peter Maydell wrote: > 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(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On Sat, Feb 26, 2022 at 06:07:15PM +0000, Peter Maydell wrote: > 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); Is exit(EXIT_FAILURE) worth using in this file? We have an inconsistent history of a magic number vs. a named constant, so either way, Reviewed-by: Eric Blake <eblake@redhat.com>
On Wed, 2 Mar 2022 at 16:31, Eric Blake <eblake@redhat.com> wrote: > Is exit(EXIT_FAILURE) worth using in this file? We have an > inconsistent history of a magic number vs. a named constant, so either > way, I'm not a huge fan of EXIT_FAILURE, I think it tends to obscure more than it helps. We have rather more 'exit(1)' than 'exit(EXIT_FAILURE)' in the codebase. -- PMM
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);
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(-)