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.mando...@gmail.com> writes:
> On Mon, Mar 15, 2021 at 1:13 PM Philippe Mathieu-Daudé <phi...@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.mando...@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
> 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?
