|
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> 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>
[Prev in Thread] | Current Thread | [Next in Thread] |