[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Thomas Huth
Subject: Re: [PATCH 2/2] util/compatfd.c: Replaced a malloc call with g_malloc.
Date: Mon, 15 Mar 2021 15:53:42 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0

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>

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...


   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

      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

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>

reply via email to

[Prev in Thread] Current Thread [Next in Thread]