| Message ID | 20210314032324.45142-6-ma.mandourr@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Replacing malloc and the like with GLib's variants | expand |
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
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; }
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(-)