On 3/15/21 3:53 PM, Thomas Huth wrote:
> 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.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
>
> 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!
I guess I misunderstood the patch description when :)
