util/compatfd.c: Fixed styling and replaced malloc

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

Posted by Mahmoud Mandour 4 years, 11 months ago

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));
 
     if (pipe(fds) == -1) {
-        free(info);
+        g_free(info);
         return -1;
     }
 
-- 
2.25.1

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

Posted by Philippe Mathieu-Daudé 4 years, 11 months ago

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;
>      }
>  
>

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

Posted by Mahmoud Mandour 4 years, 11 months ago

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

Thanks
Mahmoud

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

Posted by Markus Armbruster 4 years, 11 months ago

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

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

Posted by Thomas Huth 4 years, 11 months ago

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>

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

Posted by Philippe Mathieu-Daudé 4 years, 11 months ago

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

I guess I misunderstood the patch description when :)