Preparation for later conversion to g_auto* memory handling.

Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
---
 tests/commandhelper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/commandhelper.c b/tests/commandhelper.c
index 05e3879688..2be121ce2c 100644
--- a/tests/commandhelper.c
+++ b/tests/commandhelper.c
@@ -61,7 +61,7 @@ int main(int argc, char **argv) {
     ssize_t got;
 
     if (!log)
-        return ret;
+        goto cleanup;
 
     for (i = 1; i < argc; i++) {
         fprintf(log, "ARG:%s\n", argv[i]);
@@ -79,7 +79,7 @@ int main(int argc, char **argv) {
     }
 
     if (!(newenv = malloc(sizeof(*newenv) * n)))
-        abort();
+        goto cleanup;
 
     for (i = 0; i < n; i++) {
         newenv[i] = environ[i];
@@ -222,8 +222,10 @@ int main(int argc, char **argv) {
  cleanup:
     for (i = 0; i < G_N_ELEMENTS(buffers); i++)
         free(buffers[i]);
-    fclose(log);
-    free(newenv);
+    if (newenv)
+        free(newenv);
+    if (log)
+        fclose(log);
     return ret;
 }
 
-- 
2.26.2

On Fri, Jan 29, 2021 at 17:16:14 +0100, Tim Wiederhake wrote:
> Preparation for later conversion to g_auto* memory handling.
> 
> Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
> ---
>  tests/commandhelper.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/commandhelper.c b/tests/commandhelper.c
> index 05e3879688..2be121ce2c 100644
> --- a/tests/commandhelper.c
> +++ b/tests/commandhelper.c
> @@ -61,7 +61,7 @@ int main(int argc, char **argv) {
>      ssize_t got;
>  
>      if (!log)
> -        return ret;
> +        goto cleanup;
>  
>      for (i = 1; i < argc; i++) {
>          fprintf(log, "ARG:%s\n", argv[i]);
> @@ -79,7 +79,7 @@ int main(int argc, char **argv) {
>      }
>  
>      if (!(newenv = malloc(sizeof(*newenv) * n)))
> -        abort();
> +        goto cleanup;

Any reason for not converting this malloc to g_new directly? you get rid
of abort()/cleanup entirely.

Especially since the patches at the end of the series switch to
g_auto(ptr).

If there's a strong reason against using glibs allocators, in such case
the cleanups shouldn't be added either.

>  
>      for (i = 0; i < n; i++) {
>          newenv[i] = environ[i];
> @@ -222,8 +222,10 @@ int main(int argc, char **argv) {
>   cleanup:
>      for (i = 0; i < G_N_ELEMENTS(buffers); i++)
>          free(buffers[i]);
> -    fclose(log);
> -    free(newenv);
> +    if (newenv)
> +        free(newenv);
> +    if (log)
> +        fclose(log);
>      return ret;
>  }
>  
> -- 
> 2.26.2
> 

On Fri, Jan 29, 2021 at 18:17:36 +0100, Peter Krempa wrote:
> On Fri, Jan 29, 2021 at 17:16:14 +0100, Tim Wiederhake wrote:
> > Preparation for later conversion to g_auto* memory handling.
> > 
> > Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
> > ---
> >  tests/commandhelper.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/commandhelper.c b/tests/commandhelper.c
> > index 05e3879688..2be121ce2c 100644
> > --- a/tests/commandhelper.c
> > +++ b/tests/commandhelper.c
> > @@ -61,7 +61,7 @@ int main(int argc, char **argv) {
> >      ssize_t got;
> >  
> >      if (!log)
> > -        return ret;
> > +        goto cleanup;
> >  
> >      for (i = 1; i < argc; i++) {
> >          fprintf(log, "ARG:%s\n", argv[i]);
> > @@ -79,7 +79,7 @@ int main(int argc, char **argv) {
> >      }
> >  
> >      if (!(newenv = malloc(sizeof(*newenv) * n)))
> > -        abort();
> > +        goto cleanup;
> 
> Any reason for not converting this malloc to g_new directly? you get rid
> of abort()/cleanup entirely.
> 
> Especially since the patches at the end of the series switch to
> g_auto(ptr).
> 
> If there's a strong reason against using glibs allocators, in such case
> the cleanups shouldn't be added either.

Using glibs allocators would simplify also further patches, so if there
isn't a particular reason why you chose to use calloc it would seem
better to use g_new right away and prevent adding additional cleanup
labels.

On Fri, Jan 29, 2021 at 06:17:36PM +0100, Peter Krempa wrote:
> On Fri, Jan 29, 2021 at 17:16:14 +0100, Tim Wiederhake wrote:
> > Preparation for later conversion to g_auto* memory handling.
> > 
> > Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
> > ---
> >  tests/commandhelper.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/commandhelper.c b/tests/commandhelper.c
> > index 05e3879688..2be121ce2c 100644
> > --- a/tests/commandhelper.c
> > +++ b/tests/commandhelper.c
> > @@ -61,7 +61,7 @@ int main(int argc, char **argv) {
> >      ssize_t got;
> >  
> >      if (!log)
> > -        return ret;
> > +        goto cleanup;
> >  
> >      for (i = 1; i < argc; i++) {
> >          fprintf(log, "ARG:%s\n", argv[i]);
> > @@ -79,7 +79,7 @@ int main(int argc, char **argv) {
> >      }
> >  
> >      if (!(newenv = malloc(sizeof(*newenv) * n)))
> > -        abort();
> > +        goto cleanup;
> 
> Any reason for not converting this malloc to g_new directly? you get rid
> of abort()/cleanup entirely.

commandhelper isn't permitted to link to glib, because we need a clean
execution environment for counting FDs.

> 
> Especially since the patches at the end of the series switch to
> g_auto(ptr).
> 
> If there's a strong reason against using glibs allocators, in such case
> the cleanups shouldn't be added either.

We get away with the g_auto usage because its merely a macro

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Fri, Jan 29, 2021 at 17:52:35 +0000, Daniel Berrange wrote:
> On Fri, Jan 29, 2021 at 06:17:36PM +0100, Peter Krempa wrote:
> > On Fri, Jan 29, 2021 at 17:16:14 +0100, Tim Wiederhake wrote:
> > > Preparation for later conversion to g_auto* memory handling.
> > > 
> > > Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
> > > ---
> > >  tests/commandhelper.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tests/commandhelper.c b/tests/commandhelper.c
> > > index 05e3879688..2be121ce2c 100644
> > > --- a/tests/commandhelper.c
> > > +++ b/tests/commandhelper.c
> > > @@ -61,7 +61,7 @@ int main(int argc, char **argv) {
> > >      ssize_t got;
> > >  
> > >      if (!log)
> > > -        return ret;
> > > +        goto cleanup;
> > >  
> > >      for (i = 1; i < argc; i++) {
> > >          fprintf(log, "ARG:%s\n", argv[i]);
> > > @@ -79,7 +79,7 @@ int main(int argc, char **argv) {
> > >      }
> > >  
> > >      if (!(newenv = malloc(sizeof(*newenv) * n)))
> > > -        abort();
> > > +        goto cleanup;
> > 
> > Any reason for not converting this malloc to g_new directly? you get rid
> > of abort()/cleanup entirely.
> 
> commandhelper isn't permitted to link to glib, because we need a clean
> execution environment for counting FDs.
> 
> > 
> > Especially since the patches at the end of the series switch to
> > g_auto(ptr).
> > 
> > If there's a strong reason against using glibs allocators, in such case
> > the cleanups shouldn't be added either.
> 
> We get away with the g_auto usage because its merely a macro

Hmm, yeah. It looks a bit weird though.

On Fri, Jan 29, 2021 at 17:52:35 +0000, Daniel Berrange wrote:
> On Fri, Jan 29, 2021 at 06:17:36PM +0100, Peter Krempa wrote:
> > On Fri, Jan 29, 2021 at 17:16:14 +0100, Tim Wiederhake wrote:
> > > Preparation for later conversion to g_auto* memory handling.
> > > 
> > > Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
> > > ---
> > >  tests/commandhelper.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tests/commandhelper.c b/tests/commandhelper.c
> > > index 05e3879688..2be121ce2c 100644
> > > --- a/tests/commandhelper.c
> > > +++ b/tests/commandhelper.c
> > > @@ -61,7 +61,7 @@ int main(int argc, char **argv) {
> > >      ssize_t got;
> > >  
> > >      if (!log)
> > > -        return ret;
> > > +        goto cleanup;
> > >  
> > >      for (i = 1; i < argc; i++) {
> > >          fprintf(log, "ARG:%s\n", argv[i]);
> > > @@ -79,7 +79,7 @@ int main(int argc, char **argv) {
> > >      }
> > >  
> > >      if (!(newenv = malloc(sizeof(*newenv) * n)))
> > > -        abort();
> > > +        goto cleanup;
> > 
> > Any reason for not converting this malloc to g_new directly? you get rid
> > of abort()/cleanup entirely.
> 
> commandhelper isn't permitted to link to glib, because we need a clean
> execution environment for counting FDs.

I must say, that switching to meson made this quite fragile.

Prior to this series I see:

$ ldd ~/build/libvirt/gcc/tests/commandhelper
	linux-vdso.so.1 (0x00007ffefbf04000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f40644bb000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f40646ab000)


Now this series touches nothing related to build system and linking, yet
commiting these patches would result in:

$ ldd ~/build/libvirt/gcc/tests/commandhelper
	linux-vdso.so.1 (0x00007ffdeeda6000)
	libglib-2.0.so.0 => /lib64/libglib-2.0.so.0 (0x00007fc5190e5000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007fc5190ca000)
	libc.so.6 => /lib64/libc.so.6 (0x00007fc518eff000)
	libpcre.so.1 => /lib64/libpcre.so.1 (0x00007fc518e86000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fc518e64000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fc51923c000)

If patches 17-18 are left out we get:

$ ldd ~/build/libvirt/gcc/tests/commandhelper
	linux-vdso.so.1 (0x00007ffd9bfb7000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007fc728358000)
	libc.so.6 => /lib64/libc.so.6 (0x00007fc72818d000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fc728398000)

(libgcc_s added)

Pulling in deps automatically for stuff which is deliberately avoiding
some libraries is counterproductive here. I hope same doesn't happen
with some of the setuid binaries where we deliberately avoid libvirt.so