Re: [Qemu-devel] [PATCH v2 11/30] tests: fix hd-geo-test leaks

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  tests/hd-geo-test.c | 47 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 6176e81ab2..88f8d76d32 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -19,6 +19,8 @@
>  #include "qemu-common.h"
>  #include "libqtest.h"
>  
> +#define ARGV_SIZE 256
> +
>  static char *create_test_img(int secs)
>  {
>      char *template = strdup("/tmp/qtest.XXXXXX");
> @@ -66,7 +68,7 @@ static const CHST hd_chst[backend_last][mbr_last] = {
>      },
>  };
>  
> -static const char *img_file_name[backend_last];
> +static char *img_file_name[backend_last];
>  
>  static const CHST *cur_ide[4];
>  
> @@ -234,28 +236,34 @@ static int setup_ide(int argc, char *argv[], int 
> argv_sz,
>   */
>  static void test_ide_none(void)
>  {
> -    char *argv[256];
> +    char **argv = g_new0(char *, ARGV_SIZE);
> +    char *tmp;
>  
> -    setup_common(argv, ARRAY_SIZE(argv));
> -    qtest_start(g_strjoinv(" ", argv));
> +    setup_common(argv, ARGV_SIZE);
> +    qtest_start(tmp = g_strjoinv(" ", argv));

While I'm totally fine with things like while ((tmp = ...)) when the
result is more concise, I don't like this.  Please do

       tmp = g_strjoinv(" ", argv);
       qtest_start(tmp);

Also, please rename tmp to extra_args, argstr, or maybe args.

> +    g_strfreev(argv);
> +    g_free(tmp);
>      test_cmos();
>      qtest_end();
>  }

I think we should have a qtest_startv() that takes an argv[] and doesn't
use the shell to split arguments.  Outside this patch's scope.

[More of the same snipped...]

With the stylistic changes outlined above:
Reviewed-by: Markus Armbruster <address@hidden>