Re: [PATCH 2/2] util/compatfd.c: Replaced a malloc call with g_malloc.

[Thomas Huth]

On 15/03/2021 15.25, Markus Armbruster wrote:
> Mahmoud, it's generally a good idea to cc: people who commented on a
> previous iteration of the same patch.  In this case, Thomas.  I'm doing
> that for you now.
>
> Mahmoud Mandour <ma.mandourr@gmail.com> writes:
>
> > On Mon, Mar 15, 2021 at 1:13 PM Philippe Mathieu-Daudé <philmd@redhat.com>
> > wrote:
> >
> > > Hi Mahmoud,
> > >
> > > On 3/15/21 11:58 AM, Mahmoud Mandour wrote:
> > > > Replaced a call to malloc() and its respective call to free()
> > > > with g_malloc() and g_free().
> > > >
> > > > g_malloc() is preferred more than g_try_* functions, which
> > > > return NULL on error, when the size of the requested
> > > > allocation  is small. This is because allocating few
> > > > bytes should not be a problem in a healthy system.
> > > > Otherwise, the system is already in a critical state.
> > > >
> > > > Subsequently, removed NULL-checking after g_malloc().
> > > >
> > > > Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> > > > ---
> > > >   util/compatfd.c | 8 ++------
> > > >   1 file changed, 2 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/util/compatfd.c b/util/compatfd.c
> > > > index 174f394533..a8ec525c6c 100644
> > > > --- a/util/compatfd.c
> > > > +++ b/util/compatfd.c
> > > > @@ -72,14 +72,10 @@ static int qemu_signalfd_compat(const sigset_t *mask)
> > > >       QemuThread thread;
> > > >       int fds[2];
> > > >
> > > > -    info = malloc(sizeof(*info));
> > > > -    if (info == NULL) {
> > > > -        errno = ENOMEM;
> > > > -        return -1;
> > > > -    }
> > > > +    info = g_malloc(sizeof(*info));
> > >
> > > Watch out...
> > >
> > > https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html
> > >
> > >    If any call to allocate memory using functions g_new(), g_new0(),
> > >    g_renew(), g_malloc(), g_malloc0(), g_malloc0_n(), g_realloc(),
> > >    and g_realloc_n() fails, the application is terminated.
> > >
> > > So with your change instead of handling ENOMEM the QEMU process is
> > > simply killed.
> > >
> > > Don't you want to use g_try_new(struct sigfd_compat_info, 1) here
> > > instead?
> > >
> > > >       if (pipe(fds) == -1) {
> > > > -        free(info);
> > > > +        g_free(info);
> > > >           return -1;
> > > >       }
> > Hello Mr. Philippe,
> >
> > That's originally what I did and I sent a patch that uses a g_try_*
> > variant, and was
> > instructed by Mr. Thomas Huth that it was better to use g_malloc instead

No need to say "Mr." here ... we're not that formal on this mailing list 
here :-)

> > because this is a small allocation and the process is better killed if such
> > an allocation fails because the system is already in a very critical state
> > if it does not handle a small allocation well.
>
> You even explained this in the commit message.  Appreciated.
>
> > You can find Mr. Thomas reply to my previous patch here:
> > Re: [PATCH 5/8] util/compatfd.c: Replaced a malloc with GLib's variant
> > (gnu.org)
> > <https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05067.html>
> >
> > You can instruct me on what to do further.
>
> I figure this patch is fine.  Thomas?

Yes, looks good now, thanks for the update, Mahmoud!

Reviewed-by: Thomas Huth <thuth@redhat.com>