diff mbox

[v2,11/30] tests: fix hd-geo-test leaks

Message ID 20170221141451.28305-12-marcandre.lureau@redhat.com
State New
Headers show

Commit Message

Marc-André Lureau Feb. 21, 2017, 2:14 p.m. UTC 
Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/hd-geo-test.c | 47 +++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 16 deletions(-)

Comments

Markus Armbruster Feb. 27, 2017, 1:59 p.m. UTC | #1 
Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  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 <armbru@redhat.com>
diff mbox

Patch

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));
+    g_strfreev(argv);
+    g_free(tmp);
     test_cmos();
     qtest_end();
 }
 
 static void test_ide_mbr(bool use_device, MBRcontents mbr)
 {
-    char *argv[256];
+    char **argv = g_new0(char *, ARGV_SIZE);
+    char *tmp;
     int argc;
     Backend i;
     const char *dev;
 
-    argc = setup_common(argv, ARRAY_SIZE(argv));
+    argc = setup_common(argv, ARGV_SIZE);
     for (i = 0; i < backend_last; i++) {
         cur_ide[i] = &hd_chst[i][mbr];
         dev = use_device ? (is_hd(cur_ide[i]) ? "ide-hd" : "ide-cd") : NULL;
-        argc = setup_ide(argc, argv, ARRAY_SIZE(argv), i, dev, i, mbr, "");
+        argc = setup_ide(argc, argv, ARGV_SIZE, i, dev, i, mbr, "");
     }
-    qtest_start(g_strjoinv(" ", argv));
+    qtest_start(tmp = g_strjoinv(" ", argv));
+    g_strfreev(argv);
+    g_free(tmp);
     test_cmos();
     qtest_end();
 }
@@ -310,12 +318,13 @@  static void test_ide_device_mbr_chs(void)
 
 static void test_ide_drive_user(const char *dev, bool trans)
 {
-    char *argv[256], *opts;
+    char **argv = g_new0(char *, ARGV_SIZE);
+    char *tmp, *opts;
     int argc;
     int secs = img_secs[backend_small];
     const CHST expected_chst = { secs / (4 * 32) , 4, 32, trans };
 
-    argc = setup_common(argv, ARRAY_SIZE(argv));
+    argc = setup_common(argv, ARGV_SIZE);
     opts = g_strdup_printf("%s,%s%scyls=%d,heads=%d,secs=%d",
                            dev ?: "",
                            trans && dev ? "bios-chs-" : "",
@@ -323,11 +332,13 @@  static void test_ide_drive_user(const char *dev, bool trans)
                            expected_chst.cyls, expected_chst.heads,
                            expected_chst.secs);
     cur_ide[0] = &expected_chst;
-    argc = setup_ide(argc, argv, ARRAY_SIZE(argv),
+    argc = setup_ide(argc, argv, ARGV_SIZE,
                      0, dev ? opts : NULL, backend_small, mbr_chs,
                      dev ? "" : opts);
     g_free(opts);
-    qtest_start(g_strjoinv(" ", argv));
+    qtest_start(tmp = g_strjoinv(" ", argv));
+    g_strfreev(argv);
+    g_free(tmp);
     test_cmos();
     qtest_end();
 }
@@ -369,18 +380,21 @@  static void test_ide_device_user_chst(void)
  */
 static void test_ide_drive_cd_0(void)
 {
-    char *argv[256];
+    char **argv = g_new0(char *, ARGV_SIZE);
+    char *tmp;
     int argc, ide_idx;
     Backend i;
 
-    argc = setup_common(argv, ARRAY_SIZE(argv));
+    argc = setup_common(argv, ARGV_SIZE);
     for (i = 0; i <= backend_empty; i++) {
         ide_idx = backend_empty - i;
         cur_ide[ide_idx] = &hd_chst[i][mbr_blank];
-        argc = setup_ide(argc, argv, ARRAY_SIZE(argv),
+        argc = setup_ide(argc, argv, ARGV_SIZE,
                          ide_idx, NULL, i, mbr_blank, "");
     }
-    qtest_start(g_strjoinv(" ", argv));
+    qtest_start(tmp = g_strjoinv(" ", argv));
+    g_strfreev(argv);
+    g_free(tmp);
     test_cmos();
     qtest_end();
 }
@@ -418,6 +432,7 @@  int main(int argc, char **argv)
     for (i = 0; i < backend_last; i++) {
         if (img_file_name[i]) {
             unlink(img_file_name[i]);
+            free(img_file_name[i]);
         }
     }