Replaced a malloc() call and its respective free() call with
GLib's g_try_malloc() and g_free().

Also, did slight styling changes that were producing
style errors when using the checkpatch.pl script against
the file.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 util/compatfd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/compatfd.c b/util/compatfd.c
index ee47dd8089..834ddd0573 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -20,8 +20,7 @@
 #include <sys/syscall.h>
 #endif
 
-struct sigfd_compat_info
-{
+struct sigfd_compat_info {
     sigset_t mask;
     int fd;
 };
@@ -53,8 +52,9 @@ static void *sigwait_compat(void *opaque)
 
                 len = write(info->fd, (char *)&buffer + offset,
                             sizeof(buffer) - offset);
-                if (len == -1 && errno == EINTR)
+                if (len == -1 && errno == EINTR) {
                     continue;
+                }
 
                 if (len <= 0) {
                     return NULL;
@@ -72,14 +72,14 @@ static int qemu_signalfd_compat(const sigset_t *mask)
     QemuThread thread;
     int fds[2];
 
-    info = malloc(sizeof(*info));
+    info = g_try_malloc(sizeof(*info));
     if (info == NULL) {
         errno = ENOMEM;
         return -1;
     }
 
     if (pipe(fds) == -1) {
-        free(info);
+        g_free(info);
         return -1;
     }
 
-- 
2.25.1

On 14/03/2021 04.23, Mahmoud Mandour wrote:
> Replaced a malloc() call and its respective free() call with
> GLib's g_try_malloc() and g_free().
> 
> Also, did slight styling changes that were producing
> style errors when using the checkpatch.pl script against
> the file.

If it's unrelated, then maybe better do it in a separate patch.

> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>   util/compatfd.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/util/compatfd.c b/util/compatfd.c
> index ee47dd8089..834ddd0573 100644
> --- a/util/compatfd.c
> +++ b/util/compatfd.c
> @@ -20,8 +20,7 @@
>   #include <sys/syscall.h>
>   #endif
>   
> -struct sigfd_compat_info
> -{
> +struct sigfd_compat_info {
>       sigset_t mask;
>       int fd;
>   };
> @@ -53,8 +52,9 @@ static void *sigwait_compat(void *opaque)
>   
>                   len = write(info->fd, (char *)&buffer + offset,
>                               sizeof(buffer) - offset);
> -                if (len == -1 && errno == EINTR)
> +                if (len == -1 && errno == EINTR) {
>                       continue;
> +                }
>   
>                   if (len <= 0) {
>                       return NULL;
> @@ -72,14 +72,14 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>       QemuThread thread;
>       int fds[2];
>   
> -    info = malloc(sizeof(*info));
> +    info = g_try_malloc(sizeof(*info));
>       if (info == NULL) {
>           errno = ENOMEM;
>           return -1;
>       }

Since this is only a very small allocation, I think it would be better to 
use g_malloc() here and then simply remove the "if (info == NULL) ..." part.

  Thomas

>
> If it's unrelated, then maybe better do it in a separate patch.
>

I thought so but I didn't know whether it was a so-small change
that it didn't require its own patch or not. I will amend that.

Since this is only a very small allocation, I think it would be better to
> use g_malloc() here and then simply remove the "if (info == NULL) ..."
> part.
>

I was thinking of always maintaining the semantics of the existing
code and since g_malloc() does not behave like malloc() on
error, I refrained from using g_malloc() anywhere, but of course
I'll do it since it's the better thing to do.

I will split the patches to a two-patch series regarding the
util/compactfd.c file (one for the style change and one for
changing the malloc() call into g_malloc()) and send them
again, is that ok?

On 15/03/2021 07.43, Mahmoud Mandour wrote:
>     If it's unrelated, then maybe better do it in a separate patch.
> 
> 
> I thought so but I didn't know whether it was a so-small change
> that it didn't require its own patch or not. I will amend that.
> 
>     Since this is only a very small allocation, I think it would be better to
>     use g_malloc() here and then simply remove the "if (info == NULL) ..." part.
> 
> 
> I was thinking of always maintaining the semantics of the existing
> code and since g_malloc() does not behave like malloc() on
> error, I refrained from using g_malloc() anywhere, but of course
> I'll do it since it's the better thing to do.

Keeping the semantics is normally a good idea, but the common sense in the 
QEMU project is to rather use g_malloc() for small allocations (if 
allocating some few bytes already fails, then the system is pretty much dead 
anyway), and only g_try_malloc() for huge allocations that really might fail 
on a healthy system, too.

We should likely add some text to our coding style document to make this 
more obvious...

> I will split the patches to a two-patch series regarding the
> util/compactfd.c file (one for the style change and one for
> changing the malloc() call into g_malloc()) and send them
> again, is that ok?

Sounds good, thanks!

  Thomas

Thomas Huth <thuth@redhat.com> writes:

> On 15/03/2021 07.43, Mahmoud Mandour wrote:
>>     If it's unrelated, then maybe better do it in a separate patch.
>> I thought so but I didn't know whether it was a so-small change
>> that it didn't require its own patch or not. I will amend that.
>>     Since this is only a very small allocation, I think it would be
>> better to
>>     use g_malloc() here and then simply remove the "if (info == NULL) ..." part.
>> I was thinking of always maintaining the semantics of the existing
>> code and since g_malloc() does not behave like malloc() on
>> error, I refrained from using g_malloc() anywhere, but of course
>> I'll do it since it's the better thing to do.
>
> Keeping the semantics is normally a good idea, but the common sense in
> the QEMU project is to rather use g_malloc() for small allocations (if
> allocating some few bytes already fails, then the system is pretty
> much dead anyway), and only g_try_malloc() for huge allocations that
> really might fail on a healthy system, too.
>
> We should likely add some text to our coding style document to make
> this more obvious...

So while there are some places where we may try to dynamically scale the
memory we allocate on failure of a large allocation generally memory
allocation failure is considered fatal (ergo g_malloc, no NULL check).
However some care has to be taken depending on where we are - for
example calling abort() because something the guest did triggered us to
try an allocate more memory than we could is a no no.

We could certainly be clearer in style.rst though.

>> I will split the patches to a two-patch series regarding the
>> util/compactfd.c file (one for the style change and one for
>> changing the malloc() call into g_malloc()) and send them
>> again, is that ok?
>
> Sounds good, thanks!
>
>  Thomas


-- 
Alex Bennée