Skip to contentSkip to conversation

xl: free allocations made at top level

PATCH In review Ian Campbell 4 participants 10 messages Jul 30, 2010

lore
Share & tools
lore

Ian Campbell

author
#0 lore raw

PATCH 1/1 xl: free allocations made at top level

xl: free all allocations made at the top level

The following series makes "xl list" free of memory leaks (as reported by valgrind) subject to two exceptions outlined below.

Although we may not care about many memory leaks in xl since the commands are short lived I think it is important that we be able to use xl to validate the libxl does not leak and/or that users of libxl can be written which do not leak.

I've not extensively tested all possible xl commands so it is possible that this series may expose a latent double free etc. create, destroy, console and list seem OK.

The series applies after Ian Jackson's series consisting of
[PATCH 1/2] libxl: const-correctness for libxl_uuid2string [PATCH 2/2] xl: support "xl list <domain>"
which was posted to the list yesterday.

I used a version of valgrind which I have patched to understand the PRIVCMD_HYPERCALL ioctl on /proc/xen/privcmd. It currently understands exactly one sysctl (XEN_SYSCTL_getdomaininfolist). I will post the patch shortly and imagine I will be expanding it as I test other xl commands etc.

I ran the test with the
--leak-check=full --show-reachable=yes --track-origins=yes
options to valgrind and there are two leaks remaining:

==7040== 8 bytes in 1 blocks are still reachable in loss record 1 of 2

==7040==    at 0x4022249: calloc (vg_replace_malloc.c:467)
==7040==    by 0x40616A3: hcall_buf_prep (xc_private.c:242)
==7040==    by 0x4062306: xc_sysctl (xc_private.h:177)
==7040==    by 0x4059F61: xc_domain_getinfolist (xc_domain.c:254)
==7040==    by 0x403FDBF: libxl_list_domain (libxl.c:496)
==7040==    by 0x8054B2E: main_list (xl_cmdimpl.c:3005)
==7040==    by 0x804B2FB: main (xl.c:76)
==7040== 
==7040== 4,096 bytes in 1 blocks are still reachable in loss record 2 of 2
==7040==    at 0x40220F6: memalign (vg_replace_malloc.c:581)
==7040==    by 0x4022153: posix_memalign (vg_replace_malloc.c:709)
==7040==    by 0x4061110: xc_memalign (xc_private.c:804)
==7040==    by 0x40616D8: hcall_buf_prep (xc_private.c:250)
==7040==    by 0x4062306: xc_sysctl (xc_private.h:177)
==7040==    by 0x4059F61: xc_domain_getinfolist (xc_domain.c:254)
==7040==    by 0x403FDBF: libxl_list_domain (libxl.c:496)
==7040==    by 0x8054B2E: main_list (xl_cmdimpl.c:3005)
==7040==    by 0x804B2FB: main (xl.c:76)

These are the hypercall argument buffers which libxc allocates, locks into memory and stores in TLS using pthread_setspecific.

The memory should be freed on thread exit (libxc supplies a destructor to pthread_key_create) but since xl isn't actually using threads it never occurs.

I'm not sure that is really necessary to avoid these leaks (other than to provide an empty base line for future work). They are pretty benign since they are allocated exactly once per thread and I'm reasonably happy that in a user of libxc/libxl which used threads they would actually end up getting freed at the appropriate point.

If there were a way to tell valgrind not to worry about these allocations that would be nice, as would a palatable workaround which could be used in xl but I can't find anything suitable. For example calling pthread_exit() at the end of main() causes leaks from the C runtime so that is out. Creating a thread to do the body of the work (with main just doing pthread_join and returning the result) doesn't pass the palatable test IMHO

Ian.

author
#1 lore raw

PATCH 1/1 xl: return(N) from individual commands to top level instead of exit(N)

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1280477590 -3600
# Node ID 89a8086b0a054c10552bc32a28026773c9608772
# Parent 54981fca074262cb89fb2d1682dc6170552207b8
xl: return(N) from individual commands to top level instead of exit(N)

This allows the top level command dispatcher to cleanup some of its own allocations.

This is a fairly mechanical conversion of exit(FOO) into return FOO for the top-level command functions (i.e. main_*). There are still code paths which will exit() further down the call chains which will require actual thought.

At first glance not all the return codes are which one would normally expect for process exit codes (e.g. some are negative) but I didn't attempt to address that here.

Signed-off-by Ian Campbell <[email protected]>
tools/libxl/xl_cmdimpl.c +254 -254
diff -r 54981fca0742 -r 89a8086b0a05 tools/libxl/xl_cmdimpl.c
@@ -1701,7 +1701,7 @@ int main_memmax(int argc, char **argv)
     }
     if (optind >= argc - 1) {
         help("mem-max");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];

@@ -1710,10 +1710,10 @@ int main_memmax(int argc, char **argv)
     rc = set_memory_max(p, mem);
     if (rc) {
         fprintf(stderr, "cannot set domid %d static max memory to : %s\n", domid, mem);
-        exit(1);
-    }
-
-    exit(0);
+        return 1;
+    }
+
+    return 0;
 }
 
 void set_memory_target(char *p, char *mem)

@@ -1740,7 +1740,7 @@ int main_memset(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("mem-set");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -1748,14 +1748,14 @@ int main_memset(int argc, char **argv)
     }
     if (optind >= argc - 1) {
         help("mem-set");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];
     mem = argv[optind + 1];
 
     set_memory_target(p, mem);
-    exit(0);
+    return 0;
 }
 
 void cd_insert(char *dom, char *virtdev, char *phys)

@@ -1806,7 +1806,7 @@ int main_cd_eject(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("cd-eject");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -1814,14 +1814,14 @@ int main_cd_eject(int argc, char **argv)
     }
     if (optind >= argc - 1) {
         help("cd-eject");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];
     virtdev = argv[optind + 1];
 
     cd_insert(p, virtdev, NULL);
-    exit(0);
+    return 0;
 }
 
 int main_cd_insert(int argc, char **argv)

@@ -1833,7 +1833,7 @@ int main_cd_insert(int argc, char **argv
         switch (opt) {
         case 'h':
             help("cd-insert");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -1841,7 +1841,7 @@ int main_cd_insert(int argc, char **argv
     }
     if (optind >= argc - 2) {
         help("cd-insert");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];

@@ -1849,7 +1849,7 @@ int main_cd_insert(int argc, char **argv
     file = argv[optind + 2];
 
     cd_insert(p, virtdev, file);
-    exit(0);
+    return 0;
 }
 
 int main_console(int argc, char **argv)

@@ -1860,7 +1860,7 @@ int main_console(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("console");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -1868,7 +1868,7 @@ int main_console(int argc, char **argv)
     }
     if (optind >= argc) {
         help("console");
-        exit(2);
+        return 2;
     }
 
     find_domain(argv[optind]);

@@ -1906,7 +1906,7 @@ int main_vncviewer(int argc, char **argv
             break;
         case 'h':
             help("vncviewer");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -1915,12 +1915,12 @@ int main_vncviewer(int argc, char **argv
 
     if (argc - optind != 1) {
         help("vncviewer");
-        exit(2);
+        return 2;
     }
 
     if (vncviewer(argv[optind], autopass))
-        exit(1);
-    exit(0);
+        return 1;
+    return 0;
 }
 
 void pcilist_assignable(void)

@@ -1945,7 +1945,7 @@ int main_pcilist_assignable(int argc, ch
         switch (opt) {
         case 'h':
             help("pci-list-assignable-devices");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -1953,7 +1953,7 @@ int main_pcilist_assignable(int argc, ch
     }
 
     pcilist_assignable();
-    exit(0);
+    return 0;
 }
 
 void pcilist(char *dom)

@@ -1981,7 +1981,7 @@ int main_pcilist(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("pci-list");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -1989,13 +1989,13 @@ int main_pcilist(int argc, char **argv)
     }
     if (optind >= argc) {
         help("pci-list");
-        exit(2);
+        return 2;
     }
 
     domname = argv[optind];
 
     pcilist(domname);
-    exit(0);
+    return 0;
 }
 
 void pcidetach(char *dom, char *bdf)

@@ -2023,7 +2023,7 @@ int main_pcidetach(int argc, char **argv
         switch (opt) {
         case 'h':
             help("pci-detach");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -2031,14 +2031,14 @@ int main_pcidetach(int argc, char **argv
     }
     if (optind >= argc - 1) {
         help("pci-detach");
-        exit(2);
+        return 2;
     }
 
     domname = argv[optind];
     bdf = argv[optind + 1];
 
     pcidetach(domname, bdf);
-    exit(0);
+    return 0;
 }
 void pciattach(char *dom, char *bdf, char *vs)
 {

@@ -2065,7 +2065,7 @@ int main_pciattach(int argc, char **argv
         switch (opt) {
         case 'h':
             help("pci-attach");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -2073,7 +2073,7 @@ int main_pciattach(int argc, char **argv
     }
     if (optind >= argc - 1) {
         help("pci-attach");
-        exit(2);
+        return 2;
     }
 
     domname = argv[optind];

@@ -2083,7 +2083,7 @@ int main_pciattach(int argc, char **argv
         vs = argv[optind + 2];
 
     pciattach(domname, bdf, vs);
-    exit(0);
+    return 0;
 }
 
 void pause_domain(char *p)

@@ -2673,7 +2673,7 @@ int main_restore(int argc, char **argv)
             break;
         case 'h':
             help("restore");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -2687,7 +2687,7 @@ int main_restore(int argc, char **argv)
         checkpoint_file = argv[optind + 1];
     } else {
         help("restore");
-        exit(2);
+        return 2;
     }
 
     memset(&dom_info, 0, sizeof(dom_info));

@@ -2700,9 +2700,9 @@ int main_restore(int argc, char **argv)
 
     rc = create_domain(&dom_info);
     if (rc < 0)
-        exit(-rc);
-
-    exit(0);
+        return -rc;
+
+    return 0;
 }
 
 int main_migrate_receive(int argc, char **argv)

@@ -2714,7 +2714,7 @@ int main_migrate_receive(int argc, char 
         switch (opt) {
         case 'h':
             help("migrate-receive");
-            exit(2);
+            return 2;
             break;
         case 'e':
             daemonize = 0;

@@ -2730,10 +2730,10 @@ int main_migrate_receive(int argc, char 
 
     if (argc-optind != 0) {
         help("migrate-receive");
-        exit(2);
+        return 2;
     }
     migrate_receive(debug, daemonize);
-    exit(0);
+    return 0;
 }
 
 int main_save(int argc, char **argv)

@@ -2750,7 +2750,7 @@ int main_save(int argc, char **argv)
             break;
         case 'h':
             help("save");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -2759,14 +2759,14 @@ int main_save(int argc, char **argv)
 
     if (argc-optind < 1 || argc-optind > 3) {
         help("save");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];
     filename = argv[optind + 1];
     config_filename = argv[optind + 2];
     save_domain(p, filename, checkpoint, config_filename);
-    exit(0);
+    return 0;
 }
 
 int main_migrate(int argc, char **argv)

@@ -2782,7 +2782,7 @@ int main_migrate(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("migrate");
-            exit(0);
+            return 0;
         case 'C':
             config_filename = optarg;
             break;

@@ -2803,7 +2803,7 @@ int main_migrate(int argc, char **argv)
 
     if (argc-optind < 2 || argc-optind > 2) {
         help("migrate");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];

@@ -2816,21 +2816,21 @@ int main_migrate(int argc, char **argv)
                      ssh_command, host,
                      daemonize ? "" : " -e",
                      debug ? " -d" : "") < 0)
-            exit(1);
+            return 1;
     }
 
     migrate_domain(p, rune, config_filename);
-    exit(0);
+    return 0;
 }
 
 int main_dump_core(int argc, char **argv)
 {
     if ( argc-optind < 2 ) {
         help("dump-core");
-        exit(2);
+        return 2;
     }
     core_dump_domain(argv[optind], argv[optind + 1]);
-    exit(0);
+    return 0;
 }
 
 int main_pause(int argc, char **argv)

@@ -2843,7 +2843,7 @@ int main_pause(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("pause");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -2851,13 +2851,13 @@ int main_pause(int argc, char **argv)
     }
     if (optind >= argc) {
         help("pause");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];
 
     pause_domain(p);
-    exit(0);
+    return 0;
 }
 
 int main_unpause(int argc, char **argv)

@@ -2870,7 +2870,7 @@ int main_unpause(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("unpause");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -2878,13 +2878,13 @@ int main_unpause(int argc, char **argv)
     }
     if (optind >= argc) {
         help("unpause");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];
 
     unpause_domain(p);
-    exit(0);
+    return 0;
 }
 
 int main_destroy(int argc, char **argv)

@@ -2896,7 +2896,7 @@ int main_destroy(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("destroy");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -2904,13 +2904,13 @@ int main_destroy(int argc, char **argv)
     }
     if (optind >= argc) {
         help("destroy");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];
 
     destroy_domain(p);
-    exit(0);
+    return 0;
 }
 
 int main_shutdown(int argc, char **argv)

@@ -2922,7 +2922,7 @@ int main_shutdown(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("shutdown");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -2930,13 +2930,13 @@ int main_shutdown(int argc, char **argv)
     }
     if (optind >= argc) {
         help("shutdown");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];
 
     shutdown_domain(p);
-    exit(0);
+    return 0;
 }
 
 int main_reboot(int argc, char **argv)

@@ -2948,7 +2948,7 @@ int main_reboot(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("reboot");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -2956,13 +2956,13 @@ int main_reboot(int argc, char **argv)
     }
     if (optind >= argc) {
         help("reboot");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];
 
     reboot_domain(p);
-    exit(0);
+    return 0;
 }
 int main_list(int argc, char **argv)
 {

@@ -2991,7 +2991,7 @@ int main_list(int argc, char **argv)
             break;
         case 'h':
             help("list");
-            exit(0);
+            return 0;
         case 'v':
             verbose = 1;
             break;

@@ -3005,7 +3005,7 @@ int main_list(int argc, char **argv)
         info = libxl_list_domain(&ctx, &nb_domain);
         if (!info) {
             fprintf(stderr, "libxl_domain_infolist failed.\n");
-            exit(1);
+            return 1;
         }
         info_free = info;
     } else if (optind == argc-1) {

@@ -3013,13 +3013,13 @@ int main_list(int argc, char **argv)
         rc = libxl_domain_info(&ctx, &info_buf, domid);
         if (rc) {
             fprintf(stderr, "libxl_domain_info failed (code %d).\n", rc);
-            exit(-rc);
+            return -rc;
         }
         info = &info_buf;
         nb_domain = 1;
     } else {
         help("list");
-        exit(2);
+        return 2;
     }
 
     if (details)

@@ -3029,7 +3029,7 @@ int main_list(int argc, char **argv)
 
     free(info_free);
 
-    exit(0);
+    return 0;
 }
 
 int main_list_vm(int argc, char **argv)

@@ -3040,7 +3040,7 @@ int main_list_vm(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("list-vm");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -3048,7 +3048,7 @@ int main_list_vm(int argc, char **argv)
     }
 
     list_vm();
-    exit(0);
+    return 0;
 }
 
 int main_create(int argc, char **argv)

@@ -3091,7 +3091,7 @@ int main_create(int argc, char **argv)
             break;
         case 'h':
             help("create");
-            exit(0);
+            return 0;
         case 'n':
             dryrun = 1;
             break;

@@ -3116,7 +3116,7 @@ int main_create(int argc, char **argv)
             filename = argv[optind];
         } else {
             help("create");
-            exit(2);
+            return 2;
         }
         optind++;
     }

@@ -3134,9 +3134,9 @@ int main_create(int argc, char **argv)
 
     rc = create_domain(&dom_info);
     if (rc < 0)
-        exit(-rc);
-
-    exit(0);
+        return -rc;
+
+    return 0;
 }
 
 void button_press(char *p, char *b)

@@ -3167,7 +3167,7 @@ int main_button_press(int argc, char **a
         switch (opt) {
         case 'h':
             help("button-press");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -3175,14 +3175,14 @@ int main_button_press(int argc, char **a
     }
     if (optind >= argc - 1) {
         help("button-press");
-        exit(2);
+        return 2;
     }
 
     p = argv[optind];
     b = argv[optind + 1];
 
     button_press(p, b);
-    exit(0);
+    return 0;
 }
 
 static void print_vcpuinfo(uint32_t tdomid,

@@ -3304,7 +3304,7 @@ int main_vcpulist(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("vcpu-list");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -3312,7 +3312,7 @@ int main_vcpulist(int argc, char **argv)
     }
 
     vcpulist(argc - 2, argv + 2);
-    exit(0);
+    return 0;
 }
 
 void vcpupin(char *d, const char *vcpu, char *cpu)

@@ -3402,13 +3402,13 @@ int main_vcpupin(int argc, char **argv)
 
     if (argc != 5) {
         help("vcpu-pin");
-        exit(0);
+        return 0;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
         case 'h':
             help("vcpu-pin");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -3416,7 +3416,7 @@ int main_vcpupin(int argc, char **argv)
     }
 
     vcpupin(argv[2], argv[3] , argv[4]);
-    exit(0);
+    return 0;
 }
 
 void vcpuset(char *d, char* nr_vcpus)

@@ -3443,13 +3443,13 @@ int main_vcpuset(int argc, char **argv)
 
     if (argc != 4) {
         help("vcpu-set");
-        exit(0);
+        return 0;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
         case 'h':
         help("vcpu-set");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -3457,7 +3457,7 @@ int main_vcpuset(int argc, char **argv)
     }
 
     vcpuset(argv[2], argv[3]);
-    exit(0);
+    return 0;
 }
 
 static void output_xeninfo(void)

@@ -3564,7 +3564,7 @@ int main_info(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("info");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -3572,7 +3572,7 @@ int main_info(int argc, char **argv)
     }
 
     info();
-    exit(0);
+    return 0;
 }
 
 static int sched_credit_domain_get(

@@ -3633,7 +3633,7 @@ int main_sched_credit(int argc, char **a
             break;
         case 'h':
             help("sched-credit");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -3642,21 +3642,21 @@ int main_sched_credit(int argc, char **a
 
     if (!dom && (opt_w || opt_c)) {
         fprintf(stderr, "Must specify a domain.\n");
-        exit(1);
+        return 1;
     }
 
     if (!dom) { /* list all domain's credit scheduler info */
         info = libxl_list_domain(&ctx, &nb_domain);
         if (!info) {
             fprintf(stderr, "libxl_domain_infolist failed.\n");
-            exit(1);
+            return 1;
         }
 
         printf("%-33s %4s %6s %4s\n", "Name", "ID", "Weight", "Cap");
         for (i = 0; i < nb_domain; i++) {
             rc = sched_credit_domain_get(info[i].domid, &scinfo);
             if (rc)
-                exit(-rc);
+                return -rc;
             sched_credit_domain_output(info[i].domid, &scinfo);
         }
     } else {

@@ -3664,7 +3664,7 @@ int main_sched_credit(int argc, char **a
 
         rc = sched_credit_domain_get(domid, &scinfo);
         if (rc)
-            exit(-rc);
+            return -rc;
 
         if (!opt_w && !opt_c) { /* output credit scheduler info */
             printf("%-33s %4s %6s %4s\n", "Name", "ID", "Weight", "Cap");

@@ -3676,11 +3676,11 @@ int main_sched_credit(int argc, char **a
                 scinfo.cap = cap;
             rc = sched_credit_domain_set(domid, &scinfo);
             if (rc)
-                exit(-rc);
-        }
-    }
-
-    exit(0);
+                return -rc;
+        }
+    }
+
+    return 0;
 }
 
 int main_domid(int argc, char **argv)

@@ -3692,7 +3692,7 @@ int main_domid(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("domid");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -3703,17 +3703,17 @@ int main_domid(int argc, char **argv)
     if (!domname) {
         fprintf(stderr, "Must specify a domain name.\n\n");
         help("domid");
-        exit(1);
+        return 1;
     }
 
     if (libxl_name_to_domid(&ctx, domname, &domid)) {
         fprintf(stderr, "Can't get domid of domain name '%s', maybe this domain does not exist.\n", domname);
-        exit(1);
+        return 1;
     }
 
     printf("%d\n", domid);
 
-    exit(0);
+    return 0;
 }
 
 int main_domname(int argc, char **argv)

@@ -3726,7 +3726,7 @@ int main_domname(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("domname");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -3736,24 +3736,24 @@ int main_domname(int argc, char **argv)
     if (!argv[optind]) {
         fprintf(stderr, "Must specify a domain id.\n\n");
         help("domname");
-        exit(1);
+        return 1;
     }
     domid = strtol(argv[optind], &endptr, 10);
     if (domid == 0 && !strcmp(endptr, argv[optind])) {
         /*no digits at all*/
         fprintf(stderr, "Invalid domain id.\n\n");
-        exit(1);
+        return 1;
     }
 
     domname = libxl_domid_to_name(&ctx, domid);
     if (!domname) {
         fprintf(stderr, "Can't get domain name of domain id '%d', maybe this domain does not exist.\n", domid);
-        exit(1);
+        return 1;
     }
 
     printf("%s\n", domname);
 
-    exit(0);
+    return 0;
 }
 
 int main_rename(int argc, char **argv)

@@ -3766,7 +3766,7 @@ int main_rename(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("rename");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -3777,7 +3777,7 @@ int main_rename(int argc, char **argv)
     if (!dom || !argv[optind]) {
         fprintf(stderr, "'xl rename' requires 2 arguments.\n\n");
         help("rename");
-        exit(1);
+        return 1;
     }
 
     find_domain(dom);

@@ -3785,10 +3785,10 @@ int main_rename(int argc, char **argv)
 
     if (libxl_domain_rename(&ctx, domid, common_domname, new_name, 0)) {
         fprintf(stderr, "Can't rename domain '%s'.\n", dom);
-        exit(1);
-    }
-
-    exit(0);
+        return 1;
+    }
+
+    return 0;
 }
 
 int main_trigger(int argc, char **argv)

@@ -3803,7 +3803,7 @@ int main_trigger(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("trigger");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -3814,7 +3814,7 @@ int main_trigger(int argc, char **argv)
     if (!dom || !argv[optind]) {
         fprintf(stderr, "'xl trigger' requires between 2 and 3 arguments.\n\n");
         help("trigger");
-        exit(1);
+        return 1;
     }
 
     find_domain(dom);

@@ -3830,7 +3830,7 @@ int main_trigger(int argc, char **argv)
 
     libxl_send_trigger(&ctx, domid, trigger_name, vcpuid);
 
-    exit(0);
+    return 0;
 }
 
 

@@ -3844,7 +3844,7 @@ int main_sysrq(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("sysrq");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -3855,7 +3855,7 @@ int main_sysrq(int argc, char **argv)
     if (!dom || !argv[optind]) {
         fprintf(stderr, "'xl sysrq' requires 2 arguments.\n\n");
         help("sysrq");
-        exit(1);
+        return 1;
     }
 
     find_domain(dom);

@@ -3865,12 +3865,12 @@ int main_sysrq(int argc, char **argv)
     if (sysrq[1] != '\0') {
         fprintf(stderr, "Invalid sysrq.\n\n");
         help("sysrq");
-        exit(1);
+        return 1;
     }
 
     libxl_send_sysrq(&ctx, domid, sysrq[0]);
 
-    exit(0);
+    return 0;
 }
 
 int main_debug_keys(int argc, char **argv)

@@ -3882,7 +3882,7 @@ int main_debug_keys(int argc, char **arg
         switch (opt) {
         case 'h':
             help("debug-keys");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -3890,17 +3890,17 @@ int main_debug_keys(int argc, char **arg
     }
     if (optind >= argc) {
         help("debug-keys");
-        exit(2);
+        return 2;
     }
 
     keys = argv[optind];
 
     if (libxl_send_debug_keys(&ctx, keys)) {
         fprintf(stderr, "cannot send debug keys: %s\n", keys);
-        exit(1);
-    }
-
-    exit(0);
+        return 1;
+    }
+
+    return 0;
 }
 
 int main_dmesg(int argc, char **argv)

@@ -3917,7 +3917,7 @@ int main_dmesg(int argc, char **argv)
             break;
         case 'h':
             help("dmesg");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option not supported\n");
             break;

@@ -3933,7 +3933,7 @@ int main_dmesg(int argc, char **argv)
 
 finish:
     libxl_xen_console_read_finish(&ctx, cr);
-    exit(ret);
+    return ret;
 }
 
 int main_top(int argc, char **argv)

@@ -3944,14 +3944,14 @@ int main_top(int argc, char **argv)
         switch (opt) {
         case 'h':
             help("top");
-            exit(0);
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", opt);
-            break;
-        }
-    }
-
-    exit(system("xentop"));
+            return 0;
+        default:
+            fprintf(stderr, "option `%c' not supported.\n", opt);
+            break;
+        }
+    }
+
+    return system("xentop");
 }
 
 int main_networkattach(int argc, char **argv)

@@ -3964,22 +3964,22 @@ int main_networkattach(int argc, char **
 
     if ((argc < 3) || (argc > 11)) {
         help("network-attach");
-        exit(0);
+        return 0;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
         case 'h':
             help("network-attach");
-            exit(0);
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", opt);
-            break;
-        }
-    }
-
-    if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
-        fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
-        exit(1);
+            return 0;
+        default:
+            fprintf(stderr, "option `%c' not supported.\n", opt);
+            break;
+        }
+    }
+
+    if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
+        fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
+        return 1;
     }
     init_nic_info(&nic, -1);
     for (argv += 3, argc -= 3; argc > 0; ++argv, --argc) {

@@ -3990,7 +3990,7 @@ int main_networkattach(int argc, char **
                 nic.nictype = NICTYPE_IOEMU;
             } else {
                 fprintf(stderr, "Invalid parameter `type'.\n");
-                exit(1);
+                return 1;
             }
         } else if (!strncmp("mac=", *argv, 4)) {
             tok = strtok((*argv) + 4, ":");

@@ -3998,7 +3998,7 @@ int main_networkattach(int argc, char **
                 val = strtoul(tok, &endptr, 16);
                 if ((tok == endptr) || (val > 255)) {
                     fprintf(stderr, "Invalid parameter `mac'.\n");
-                    exit(1);
+                    return 1;
                 }
                 nic.mac[i] = val;
             }

@@ -4007,7 +4007,7 @@ int main_networkattach(int argc, char **
         } else if (!strncmp("ip=", *argv, 3)) {
             if (!inet_aton((*argv) + 3, &(nic.ip))) {
                 fprintf(stderr, "Invalid parameter `ip'.\n");
-                exit(1);
+                return 1;
             }
         } else if (!strncmp("script=", *argv, 6)) {
             nic.script = (*argv) + 6;

@@ -4015,7 +4015,7 @@ int main_networkattach(int argc, char **
             val = strtoul((*argv) + 8, &endptr, 10);
             if (((*argv) + 8) == endptr) {
                 fprintf(stderr, "Invalid parameter `backend'.\n");
-                exit(1);
+                return 1;
             }
             nic.backend_domid = val;
         } else if (!strncmp("vifname=", *argv, 8)) {

@@ -4026,15 +4026,15 @@ int main_networkattach(int argc, char **
         } else if (!strncmp("accel=", *argv, 6)) {
         } else {
             fprintf(stderr, "unrecognized argument `%s'\n", *argv);
-            exit(1);
+            return 1;
         }
     }
     nic.domid = domid;
     if (libxl_device_nic_add(&ctx, domid, &nic)) {
         fprintf(stderr, "libxl_device_nic_add failed.\n");
-        exit(1);
-    }
-    exit(0);
+        return 1;
+    }
+    return 0;
 }
 
 int main_networklist(int argc, char **argv)

@@ -4045,13 +4045,13 @@ int main_networklist(int argc, char **ar
 
     if (argc < 3) {
         help("network-list");
-        exit(1);
+        return 1;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
             case 'h':
                 help("network-list");
-                exit(0);
+                return 0;
             default:
                 fprintf(stderr, "option `%c' not supported.\n", opt);
                 break;

@@ -4082,7 +4082,7 @@ int main_networklist(int argc, char **ar
                    nics->rref_tx, nics->rref_rx, nics->backend);
         }
     }
-    exit(0);
+    return 0;
 }
 
 int main_networkdetach(int argc, char **argv)

@@ -4092,40 +4092,40 @@ int main_networkdetach(int argc, char **
 
     if (argc != 4) {
         help("network-detach");
-        exit(0);
+        return 0;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
         case 'h':
             help("network-detach");
-            exit(0);
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", opt);
-            break;
-        }
-    }
-
-    if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
-        fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
-        exit(1);
+            return 0;
+        default:
+            fprintf(stderr, "option `%c' not supported.\n", opt);
+            break;
+        }
+    }
+
+    if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
+        fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
+        return 1;
     }
 
     if (!strchr(argv[3], ':')) {
         if (libxl_devid_to_device_nic(&ctx, domid, argv[3], &nic)) {
             fprintf(stderr, "Unknown device %s.\n", argv[3]);
-            exit(1);
+            return 1;
         }
     } else {
         if (libxl_mac_to_device_nic(&ctx, domid, argv[3], &nic)) {
             fprintf(stderr, "Unknown device %s.\n", argv[3]);
-            exit(1);
+            return 1;
         }
     }
     if (libxl_device_nic_del(&ctx, &nic, 1)) {
         fprintf(stderr, "libxl_device_nic_del failed.\n");
-        exit(1);
-    }
-    exit(0);
+        return 1;
+    }
+    return 0;
 }
 
 int main_blockattach(int argc, char **argv)

@@ -4137,13 +4137,13 @@ int main_blockattach(int argc, char **ar
 
     if ((argc < 5) || (argc > 7)) {
         help("block-attach");
-        exit(0);
+        return 0;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
         case 'h':
             help("block-attach");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4167,16 +4167,16 @@ int main_blockattach(int argc, char **ar
             disk.phystype = PHYSTYPE_QCOW2;
         } else {
             fprintf(stderr, "Error: `%s' is not a valid disk image.\n", tok);
-            exit(1);
+            return 1;
         }
     } else {
         fprintf(stderr, "Error: `%s' is not a valid block device.\n", tok);
-        exit(1);
+        return 1;
     }
     disk.physpath = strtok(NULL, "\0");
     if (!disk.physpath) {
         fprintf(stderr, "Error: missing path to disk image.\n");
-        exit(1);
+        return 1;
     }
     disk.virtpath = argv[4];
     disk.unpluggable = 1;

@@ -4184,12 +4184,12 @@ int main_blockattach(int argc, char **ar
 
     if (domain_qualifier_to_domid(argv[2], &fe_domid, 0) < 0) {
         fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
-        exit(1);
+        return 1;
     }
     if (argc == 7) {
         if (domain_qualifier_to_domid(argv[6], &be_domid, 0) < 0) {
             fprintf(stderr, "%s is an invalid domain identifier\n", argv[6]);
-            exit(1);
+            return 1;
         }
     }
     disk.domid = fe_domid;

@@ -4197,7 +4197,7 @@ int main_blockattach(int argc, char **ar
     if (libxl_device_disk_add(&ctx, fe_domid, &disk)) {
         fprintf(stderr, "libxl_device_disk_add failed.\n");
     }
-    exit(0);
+    return 0;
 }
 
 int main_blocklist(int argc, char **argv)

@@ -4209,13 +4209,13 @@ int main_blocklist(int argc, char **argv
 
     if (argc < 3) {
         help("block-list");
-        exit(0);
+        return 0;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
         case 'h':
             help("block-list");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4242,7 +4242,7 @@ int main_blocklist(int argc, char **argv
             }
         }
     }
-    exit(0);
+    return 0;
 }
 
 int main_blockdetach(int argc, char **argv)

@@ -4252,31 +4252,31 @@ int main_blockdetach(int argc, char **ar
 
     if (argc != 4) {
         help("block-detach");
-        exit(0);
+        return 0;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
         case 'h':
             help("block-detach");
-            exit(0);
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", opt);
-            break;
-        }
-    }
-
-    if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
-        fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
-        exit(1);
+            return 0;
+        default:
+            fprintf(stderr, "option `%c' not supported.\n", opt);
+            break;
+        }
+    }
+
+    if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
+        fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
+        return 1;
     }
     if (libxl_devid_to_device_disk(&ctx, domid, argv[3], &disk)) {
         fprintf(stderr, "Error: Device %s not connected.\n", argv[3]);
-        exit(1);
+        return 1;
     }
     if (libxl_device_disk_del(&ctx, &disk, 1)) {
         fprintf(stderr, "libxl_device_del failed.\n");
     }
-    exit(0);
+    return 0;
 }
 
 int main_network2attach(int argc, char **argv)

@@ -4290,13 +4290,13 @@ int main_network2attach(int argc, char *
 
     if ((argc < 3) || (argc > 12)) {
         help("network2-attach");
-        exit(0);
+        return 0;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
         case 'h':
             help("network2-attach");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4305,7 +4305,7 @@ int main_network2attach(int argc, char *
 
     if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
         fprintf(stderr, "%s is an invalid domain identifier\n", argv[1]);
-        exit(1);
+        return 1;
     }
     init_net2_info(&net2, -1);
     for (argv += 3, argc -= 3; argc > 0; --argc, ++argv) {

@@ -4315,7 +4315,7 @@ int main_network2attach(int argc, char *
                 val = strtoul(tok, &endptr, 16);
                 if ((tok == endptr) || (val > 255)) {
                     fprintf(stderr, "Invalid parameter `front_mac'.\n");
-                    exit(1);
+                    return 1;
                 }
                 net2.front_mac[i] = val;
             }

@@ -4325,7 +4325,7 @@ int main_network2attach(int argc, char *
                 val = strtoul(tok, &endptr, 16);
                 if ((tok == endptr) || (val > 255)) {
                     fprintf(stderr, "Invalid parameter back_mac=%s.\n", *argv + 9);
-                    exit(1);
+                    return 1;
                 }
                 net2.back_mac[i] = val;
             }

@@ -4345,26 +4345,26 @@ int main_network2attach(int argc, char *
             val = strtoul(*argv + 5, &endptr, 10);
             if (endptr == (*argv + 5)) {
                 fprintf(stderr, "Invalid parameter pdev=%s.\n", *argv + 5);
-                exit(1);
+                return 1;
             }
             net2.pdev = val;
         } else if (!strncmp("max_bypasses=", *argv, 13)) {
             val = strtoul(*argv + 13, &endptr, 10);
             if (endptr == (*argv + 13)) {
                 fprintf(stderr, "Invalid parameter max_bypasses=%s.\n", *argv + 13);
-                exit(1);
+                return 1;
             }
             net2.max_bypasses = val;
         } else {
             fprintf(stderr, "unrecognized argument `%s'\n", *argv);
-            exit(1);
+            return 1;
         }
     }
 
     if (back_dom) {
         if (domain_qualifier_to_domid(back_dom, &back_domid, 0) < 0) {
             fprintf(stderr, "%s is an invalid domain identifier\n", back_dom);
-            exit(1);
+            return 1;
         }
     }
     net2.domid = domid;

@@ -4372,7 +4372,7 @@ int main_network2attach(int argc, char *
     if (libxl_device_net2_add(&ctx, domid, &net2)) {
         fprintf(stderr, "libxl_device_net2_add failed.\n");
     }
-    exit(0);
+    return 0;
 }
 
 int main_network2list(int argc, char **argv)

@@ -4383,13 +4383,13 @@ int main_network2list(int argc, char **a
 
     if (argc < 3) {
         help("network2-list");
-        exit(0);
+        return 0;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
         case 'h':
             help("network2-list");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4417,7 +4417,7 @@ int main_network2list(int argc, char **a
             }
         }
     }
-    exit(0);
+    return 0;
 }
 
 int main_network2detach(int argc, char **argv)

@@ -4427,32 +4427,32 @@ int main_network2detach(int argc, char *
 
     if (argc != 4) {
         help("network2-detach");
-        exit(0);
+        return 0;
     }
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
         case 'h':
             help("network2-detach");
-            exit(0);
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", opt);
-            break;
-        }
-    }
-
-    if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
-        fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
-        exit(1);
+            return 0;
+        default:
+            fprintf(stderr, "option `%c' not supported.\n", opt);
+            break;
+        }
+    }
+
+    if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
+        fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
+        return 1;
     }
     if (libxl_devid_to_device_net2(&ctx, domid, argv[3], &net2)) {
        fprintf(stderr, "Error: Device %s not connected.\n", argv[3]);
-        exit(1);
+        return 1;
     }
     if (libxl_device_net2_del(&ctx, &net2, 1)) {
         fprintf(stderr, "libxl_device_net2_del failed.\n");
-        exit(1);
-    }
-    exit(0);
+        return 1;
+    }
+    return 0;
 }
 
 static char *uptime_to_string(unsigned long time, int short_mode)

@@ -4624,7 +4624,7 @@ int main_uptime(int argc, char **argv)
             break;
         case 'h':
             help("uptime");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4638,7 +4638,7 @@ int main_uptime(int argc, char **argv)
 
     print_uptime(short_mode, domains, nb_doms);
 
-    exit(0);
+    return 0;
 }
 
 int main_tmem_list(int argc, char **argv)

@@ -4659,7 +4659,7 @@ int main_tmem_list(int argc, char **argv
             break;
         case 'h':
             help("tmem-list");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4670,7 +4670,7 @@ int main_tmem_list(int argc, char **argv
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-list");
-        exit(1);
+        return 1;
     }
 
     if (all)

@@ -4680,11 +4680,11 @@ int main_tmem_list(int argc, char **argv
 
     buf = libxl_tmem_list(&ctx, domid, use_long);
     if (buf == NULL)
-        exit(-1);
+        return -1;
 
     printf("%s\n", buf);
     free(buf);
-    exit(0);
+    return 0;
 }
 
 int main_tmem_freeze(int argc, char **argv)

@@ -4700,7 +4700,7 @@ int main_tmem_freeze(int argc, char **ar
             break;
         case 'h':
             help("tmem-freeze");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4711,7 +4711,7 @@ int main_tmem_freeze(int argc, char **ar
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-freeze");
-        exit(1);
+        return 1;
     }
 
     if (all)

@@ -4720,7 +4720,7 @@ int main_tmem_freeze(int argc, char **ar
         find_domain(dom);
 
     libxl_tmem_freeze(&ctx, domid);
-    exit(0);
+    return 0;
 }
 
 int main_tmem_destroy(int argc, char **argv)

@@ -4736,7 +4736,7 @@ int main_tmem_destroy(int argc, char **a
             break;
         case 'h':
             help("tmem-destroy");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4747,7 +4747,7 @@ int main_tmem_destroy(int argc, char **a
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-destroy");
-        exit(1);
+        return 1;
     }
 
     if (all)

@@ -4756,7 +4756,7 @@ int main_tmem_destroy(int argc, char **a
         find_domain(dom);
 
     libxl_tmem_destroy(&ctx, domid);
-    exit(0);
+    return 0;
 }
 
 int main_tmem_thaw(int argc, char **argv)

@@ -4772,7 +4772,7 @@ int main_tmem_thaw(int argc, char **argv
             break;
         case 'h':
             help("tmem-thaw");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4783,7 +4783,7 @@ int main_tmem_thaw(int argc, char **argv
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-thaw");
-        exit(1);
+        return 1;
     }
 
     if (all)

@@ -4792,7 +4792,7 @@ int main_tmem_thaw(int argc, char **argv
         find_domain(dom);
 
     libxl_tmem_thaw(&ctx, domid);
-    exit(0);
+    return 0;
 }
 
 int main_tmem_set(int argc, char **argv)

@@ -4822,7 +4822,7 @@ int main_tmem_set(int argc, char **argv)
             break;
         case 'h':
             help("tmem-set");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4833,7 +4833,7 @@ int main_tmem_set(int argc, char **argv)
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-set");
-        exit(1);
+        return 1;
     }
 
     if (all)

@@ -4844,7 +4844,7 @@ int main_tmem_set(int argc, char **argv)
     if (!opt_w && !opt_c && !opt_p) {
         fprintf(stderr, "No set value specified.\n\n");
         help("tmem-set");
-        exit(1);
+        return 1;
     }
 
     if (opt_w)

@@ -4854,7 +4854,7 @@ int main_tmem_set(int argc, char **argv)
     if (opt_p)
         libxl_tmem_set(&ctx, domid, "compress", compress);
 
-    exit(0);
+    return 0;
 }
 
 int main_tmem_shared_auth(int argc, char **argv)

@@ -4880,7 +4880,7 @@ int main_tmem_shared_auth(int argc, char
             break;
         case 'h':
             help("tmem-shared-auth");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4891,7 +4891,7 @@ int main_tmem_shared_auth(int argc, char
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-shared-auth");
-        exit(1);
+        return 1;
     }
 
     if (all)

@@ -4902,18 +4902,18 @@ int main_tmem_shared_auth(int argc, char
     if (uuid == NULL || autharg == NULL) {
         fprintf(stderr, "No uuid or auth specified.\n\n");
         help("tmem-shared-auth");
-        exit(1);
+        return 1;
     }
 
     auth = strtol(autharg, &endptr, 10);
     if (*endptr != '\0') {
         fprintf(stderr, "Invalid auth, valid auth are <0|1>.\n\n");
-        exit(1);
+        return 1;
     }
 
     libxl_tmem_shared_auth(&ctx, domid, uuid, auth);
 
-    exit(0);
+    return 0;
 }
 
 int main_tmem_freeable(int argc, char **argv)

@@ -4925,7 +4925,7 @@ int main_tmem_freeable(int argc, char **
         switch (opt) {
         case 'h':
             help("tmem-freeable");
-            exit(0);
+            return 0;
         default:
             fprintf(stderr, "option `%c' not supported.\n", opt);
             break;

@@ -4934,8 +4934,8 @@ int main_tmem_freeable(int argc, char **
 
     mb = libxl_tmem_freeable(&ctx);
     if (mb == -1)
-        exit(-1);
+        return -1;
 
     printf("%d\n", mb);
-    exit(0);
-}
+    return 0;
+}
author
#2 lore raw

PATCH 1/1 xl: free the libxl context before exit

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1280477591 -3600
# Node ID 8b2d8f18090171e161c398019bacedbb5f43455b # Parent 89a8086b0a054c10552bc32a28026773c9608772
xl: free the libxl context before exit

Signed-off-by Ian Campbell <[email protected]>
tools/libxl/xl.c +8 -3
diff -r 89a8086b0a05 -r 8b2d8f180901 tools/libxl/xl.c
@@ -40,6 +40,7 @@ int main(int argc, char **argv)
     int opt = 0;
     char *cmd = 0;
     struct cmd_spec *cspec;
+    int ret;
 
     while ((opt = getopt(argc, argv, "+v")) >= 0) {
         switch (opt) {

@@ -72,12 +73,16 @@ int main(int argc, char **argv)
 
     cspec = cmdtable_lookup(cmd);
     if (cspec)
-        return cspec->cmd_impl(argc, argv);
+        ret = cspec->cmd_impl(argc, argv);
     else if (!strcmp(cmd, "help")) {
         help(argv[optind]);
-        exit(0);
+        ret = 0;
     } else {
         fprintf(stderr, "command not implemented\n");
-        exit(1);
+        ret = 1;
     }
+
+    libxl_ctx_free(&ctx);
+
+    return ret;
 }
author
#3 lore raw

PATCH 1/1 xl: destroy the logger before exit

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1280477591 -3600
# Node ID f5f5949d98f0104ad1422ddacded20875f23d38d # Parent 8b2d8f18090171e161c398019bacedbb5f43455b
xl: destroy the logger before exit

Signed-off-by Ian Campbell <[email protected]>
tools/libxl/xl.c +1 -0
diff -r 8b2d8f180901 -r f5f5949d98f0 tools/libxl/xl.c
@@ -83,6 +83,7 @@ int main(int argc, char **argv)
     }
 
     libxl_ctx_free(&ctx);
+    xtl_logger_destroy((xentoollog_logger*)logger);
 
     return ret;
 }
#4 lore raw
In reply to Ian Campbell
Quoted 8 lines · Ian Campbell

If there were a way to tell valgrind not to worry about these allocations that would be nice, as would a palatable workaround which could be used in xl but I can't find anything suitable. For example calling pthread_exit() at the end of main() causes leaks from the C runtime so that is out. Creating a thread to do the body of the work (with main just doing pthread_join and returning the result) doesn't pass the palatable test IMHO

This seems to work. I'm not entirely sure about it though -- it's not clear what happens if the thread which calls xc_interface_close is not the final thread to exit and whether we would still leak the buffer from the thread which does the final exit() call.

It's enough to make xl valgrind clean though, so perhaps that is enough.

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1280481944 -3600
# Node ID b8bf3e732b9a4b3a1614dff87af48560d3839e3b
# Parent f5f5949d98f0104ad1422ddacded20875f23d38d
libxc: free thread specific hypercall buffer on xc_interface_close

The per-thread hypercall buffer is usually cleaned up on pthread_exit by the destructor passed to pthread_key_create. However if the calling application is not threaded then the destructor is never called.

This frees the data for the current thread only but that is OK since any other threads will be cleaned up by the destructor.

Signed-off-by Ian Campbell <[email protected]>
tools/libxc/xc_private.c +15 -0
diff -r f5f5949d98f0 -r b8bf3e732b9a tools/libxc/xc_private.c
@@ -57,6 +57,8 @@ xc_interface *xc_interface_open(xentooll
     return 0;
 }
 
+static void xc_clean_hcall_buf(void);
+
 int xc_interface_close(xc_interface *xch)
 {
     int rc = 0;

@@ -68,6 +70,9 @@ int xc_interface_close(xc_interface *xch
         rc = xc_interface_close_core(xch, xch->fd);
         if (rc) PERROR("Could not close hypervisor interface");
     }
+
+    xc_clean_hcall_buf();
+
     free(xch);
     return rc;
 }

@@ -180,6 +185,8 @@ int hcall_buf_prep(void **addr, size_t l
 int hcall_buf_prep(void **addr, size_t len) { return 0; }
 void hcall_buf_release(void **addr, size_t len) { }
 
+static void xc_clean_hcall_buf(void) { }
+
 #else /* !__sun__ */
 
 int lock_pages(void *addr, size_t len)

@@ -223,6 +230,14 @@ static void _xc_clean_hcall_buf(void *m)
     }
 
     pthread_setspecific(hcall_buf_pkey, NULL);
+}
+
+static void xc_clean_hcall_buf(void)
+{
+    void *hcall_buf = pthread_getspecific(hcall_buf_pkey);
+
+    if (hcall_buf)
+        _xc_clean_hcall_buf(hcall_buf);
 }
 
 static void _xc_init_hcall_buf(void)
#5 lore raw

Ian Campbell writes ("[Xen-devel] Re: [PATCH 0 of 3] xl: free allocations made at top level"):

Quoted 4 lines

This seems to work. I'm not entirely sure about it though -- it's not clear what happens if the thread which calls xc_interface_close is not the final thread to exit and whether we would still leak the buffer from the thread which does the final exit() call.

Yers. We don't impose a requirement that the thread that calls xc_interface_open has to be the one which calls _close, so you can still leak the buffer from the original thread.

OTOH if you don't want memory leaks you do need to do xc_interface_close. So perhaps we should have a reference count or something ? Is it even possible to destroy thread-specific data for another thread ?

Also, while I was reading this code I noticed this:

    pthread_once(&hcall_buf_pkey_once, _xc_init_hcall_buf);

and:

    static void _xc_init_hcall_buf(void)
    {
        pthread_key_create(&hcall_buf_pkey, _xc_clean_hcall_buf);
    }

It seems to me that this is possibly racy, if two threads call hcall_buf_prep simultaneously for the first time (which is not at all implausible in some possible scenarios). pthread_once doesn't seem to guarantee that the first two calls arrive at once, the 2nd call doesn't return before the first call has even entered the user function. So the thread which loses the race can read an uninitialised hcall_buf_pkey.

Ian.

#6 lore raw
Quoted 9 lines · Ian Jackson

Ian Campbell writes ("[Xen-devel] Re: [PATCH 0 of 3] xl: free allocations made at top level"):

Quoted 4 lines

This seems to work. I'm not entirely sure about it though -- it's not clear what happens if the thread which calls xc_interface_close is not the final thread to exit and whether we would still leak the buffer from the thread which does the final exit() call.

Yers. We don't impose a requirement that the thread that calls xc_interface_open has to be the one which calls _close, so you can still leak the buffer from the original thread.

Unless that exists with pthread_exit, I suppose.

TBH I think we needn't worry too much about this particular leak -- for threads which do come and go they data will be freed, and for the last thread which exists it's not really a worry.

The patch is enough to make a single threaded user like xl leak free which is useful for the purposes of spotting any more serious leaks.

Quoted 3 lines

OTOH if you don't want memory leaks you do need to do xc_interface_close. So perhaps we should have a reference count or something ?

As long as the handle is either opened once and closed on exit or each thread opens and closes its own handle I think everything is ok.

Quoted 2 lines

Is it even possible to destroy thread-specific data for
another thread ?

I don't think so, but I don't think its a concern in this case since all threads will either clean up after themselves when they are done or the whole process is exiting anyway.

It's also fine to free the thread-specific data too eagerly, worst case the next attempts at a hypercall will have to reallocate it.

Quoted 11 lines

Also, while I was reading this code I noticed this:

    pthread_once(&hcall_buf_pkey_once, _xc_init_hcall_buf);

and:

    static void _xc_init_hcall_buf(void)
    {
        pthread_key_create(&hcall_buf_pkey, _xc_clean_hcall_buf);
    }
Quoted 7 lines

It seems to me that this is possibly racy, if two threads call hcall_buf_prep simultaneously for the first time (which is not at all implausible in some possible scenarios). pthread_once doesn't seem to guarantee that the first two calls arrive at once, the 2nd call doesn't return before the first call has even entered the user function. So the thread which loses the race can read an uninitialised hcall_buf_pkey.

Looking at the glibc source code it looks like the loser of the race will wait, judging from the comment "/* Somebody else got here first. Wait. */" and the subsequent sys_futex call in the assembly (without studying it too hard).

I'm not sure pthread_once would be at all useful without these semantics. I guess that doesn't mean those aren't the specified semantics though ;-)

Ian.

Acked-by
#7 lore raw

Ian Campbell writes ("Re: [Xen-devel] Re: [PATCH 0 of 3] xl: free allocations made at top level"):

Quoted 2 lines

The patch is enough to make a single threaded user like xl leak free which is useful for the purposes of spotting any more serious leaks.

I think this is the right way to think about it, so I guess we should apply your patch. But I'll wait for an opinion from Stefano in case he has a different perspective.

So:

Acked-by Ian Jackson <[email protected]>
Quoted 3 lines

I'm not sure pthread_once would be at all useful without these semantics. I guess that doesn't mean those aren't the specified semantics though ;-)

Quite :-). But in fact SuSv3 says

| On return from pthread_once(), init_routine shall have completed

So it's OK.

Ian.

Acked-by
#8 lore raw
Quoted 11 lines · Ian Jackson

Ian Campbell writes ("Re: [Xen-devel] Re: [PATCH 0 of 3] xl: free allocations made at top level"):

Quoted 2 lines

The patch is enough to make a single threaded user like xl leak free which is useful for the purposes of spotting any more serious leaks.

I think this is the right way to think about it, so I guess we should apply your patch. But I'll wait for an opinion from Stefano in case he has a different perspective.

So:

Acked-by Ian Jackson <[email protected]>

I think this patch series should be applied.

Acked-by Stefano Stabellini <[email protected]>
#9 lore raw
In reply to Ian Campbell
Quoted 6 lines · Ian Campbell

I used a version of valgrind which I have patched to understand the PRIVCMD_HYPERCALL ioctl on /proc/xen/privcmd. It currently understands exactly one sysctl (XEN_SYSCTL_getdomaininfolist). I will post the patch shortly and imagine I will be expanding it as I test other xl commands etc.

Here is the patch I used. It's obviously very basic and terribly incomplete, only understands a single sysctl, only for the version of Xen it is built against, is my first attempt at hacking valgrind, etc etc.

I also bailed on the normal valgrind policy of duplicating the interface headers so usage is:

	./configure --with-xen=/path/to/headers

The path needs to be to an installed set of xen headers, such that "#include <xen/xen.h>" is valid e.g. dist/install/usr/include in your built Xen tree or /usr/include or something equivalent (e.g. libxen-dev installed under Debian etc.)

Ian.

configure.in +5 -0
index 62e1837..e71ecd6 100644
@@ -1558,6 +1558,11 @@ elif test x$VGCONF_PLATFORM_SEC_CAPS = xPPC32_AIX5 ; then
   mflag_secondary=-q32
 fi
 
+AC_ARG_WITH(xen,
+   [  --with-xen=             Specify location of Xen headers],
+   XEN_CFLAGS=-I$withval
+)
+AC_SUBST(XEN_CFLAGS)
 
 AC_ARG_WITH(mpicc,
    [  --with-mpicc=           Specify name of MPI2-ised C compiler],
coregrind/Makefile.am +3 -1
index d9d1bca..d7216f9 100644
@@ -211,6 +211,7 @@ noinst_HEADERS = \
 	m_syswrap/priv_syswrap-aix5.h \
 	m_syswrap/priv_syswrap-darwin.h \
 	m_syswrap/priv_syswrap-main.h \
+	m_syswrap/priv_syswrap-xen.h \
 	m_ume/priv_ume.h
 
 #----------------------------------------------------------------------------

@@ -338,6 +339,7 @@ COREGRIND_SOURCES_COMMON = \
 	m_syswrap/syswrap-ppc64-aix5.c \
 	m_syswrap/syswrap-x86-darwin.c \
 	m_syswrap/syswrap-amd64-darwin.c \
+	m_syswrap/syswrap-xen.c \
 	m_ume/elf.c \
 	m_ume/macho.c \
 	m_ume/main.c \

@@ -350,7 +352,7 @@ nodist_libcoregrind_@VGCONF_ARCH_PRI@_@VGCONF_OS@_a_SOURCES = \
 libcoregrind_@VGCONF_ARCH_PRI@_@VGCONF_OS@_a_CPPFLAGS = \
     $(AM_CPPFLAGS_@VGCONF_PLATFORM_PRI_CAPS@)
 libcoregrind_@VGCONF_ARCH_PRI@_@VGCONF_OS@_a_CFLAGS = \
-    $(AM_CFLAGS_@VGCONF_PLATFORM_PRI_CAPS@)
+    $(AM_CFLAGS_@VGCONF_PLATFORM_PRI_CAPS@) @XEN_CFLAGS@
 libcoregrind_@VGCONF_ARCH_PRI@_@VGCONF_OS@_a_CCASFLAGS = \
     $(AM_CCASFLAGS_@VGCONF_PLATFORM_PRI_CAPS@)
 if VGCONF_HAVE_PLATFORM_SEC
coregrind/m_syswrap/priv_syswrap-xen.h new file +10 -0
new file mode 100644
index 0000000..c65edca
@@ -0,0 +1,10 @@
+#ifndef __PRIV_SYSWRAP_XEN_H
+#define __PRIV_SYSWRAP_XEN_H
+
+DECL_TEMPLATE(xen, ioctl_privcmd_hypercall);
+
+#endif   // __PRIV_SYSWRAP_XEN_H
+
+/*--------------------------------------------------------------------*/
+/*--- end                                                          ---*/
+/*--------------------------------------------------------------------*/
coregrind/m_syswrap/syswrap-linux.c +10 -1
index 247402d..42dc4d9 100644
@@ -57,7 +57,7 @@
 #include "priv_types_n_macros.h"
 #include "priv_syswrap-generic.h"
 #include "priv_syswrap-linux.h"
-
+#include "priv_syswrap-xen.h"
 
 // Run a thread from beginning to end and return the thread's
 // scheduler-return-code.

@@ -4821,6 +4821,11 @@ PRE(sys_ioctl)
       }
       break;
 
+
+   case VKI_XEN_IOCTL_PRIVCMD_HYPERCALL:
+      WRAPPER_PRE_NAME(xen, ioctl_privcmd_hypercall)(tid, layout, arrghs, status, flags);
+      break;
+
    default:
       /* EVIOC* are variable length and return size written on success */
       switch (ARG2 & ~(_VKI_IOC_SIZEMASK << _VKI_IOC_SIZESHIFT)) {

@@ -5633,6 +5638,10 @@ POST(sys_ioctl)
       }
       break;
 
+   case VKI_XEN_IOCTL_PRIVCMD_HYPERCALL:
+      WRAPPER_POST_NAME(xen, ioctl_privcmd_hypercall)(tid, arrghs, status);
+      break;
+
    default:
       /* EVIOC* are variable length and return size written on success */
       switch (ARG2 & ~(_VKI_IOC_SIZEMASK << _VKI_IOC_SIZESHIFT)) {
coregrind/m_syswrap/syswrap-xen.c new file +117 -0

117 lines not on this page · raw message

include/Makefile.am +2 -1
index 33d0857..22bffa7 100644
@@ -54,7 +54,8 @@ nobase_pkginclude_HEADERS = \
 	vki/vki-scnums-ppc64-linux.h	\
 	vki/vki-scnums-x86-linux.h	\
 	vki/vki-scnums-arm-linux.h	\
-	vki/vki-scnums-darwin.h
+	vki/vki-scnums-darwin.h		
+	vki/vki-xen.h
 
 noinst_HEADERS = \
 	vki/vki-ppc32-aix5.h		\
include/pub_tool_vki.h +1 -0
index 73a4174..c4c117f 100644
@@ -47,6 +47,7 @@
 
 #if defined(VGO_linux)
 #  include "vki/vki-linux.h"
+#  include "vki/vki-xen.h"
 #elif defined(VGP_ppc32_aix5)
 #  include "vki/vki-ppc32-aix5.h"
 #elif defined(VGP_ppc64_aix5)
include/vki/vki-linux.h +11 -0
index beff378..86126ea 100644
@@ -2709,6 +2709,17 @@ struct vki_getcpu_cache {
 #define VKI_EV_MAX		0x1f
 #define VKI_EV_CNT		(VKI_EV_MAX+1)
 
+//----------------------------------------------------------------------
+// Xen privcmd IOCTL
+//----------------------------------------------------------------------
+
+struct vki_xen_privcmd_hypercall {
+	__vki_u64 op;
+	__vki_u64 arg[5];
+};
+
+#define VKI_XEN_IOCTL_PRIVCMD_HYPERCALL _VKI_IOC(_VKI_IOC_NONE, 'P', 0, sizeof(struct vki_xen_privcmd_hypercall))
+
 #endif // __VKI_LINUX_H
 
 /*--------------------------------------------------------------------*/
include/vki/vki-xen.h new file +8 -0
new file mode 100644
index 0000000..7842cc9
@@ -0,0 +1,8 @@
+#ifndef __VKI_XEN_H
+#define __VKI_XEN_H
+
+#endif // __VKI_XEN_H
+
+/*--------------------------------------------------------------------*/
+/*--- end                                                          ---*/
+/*--------------------------------------------------------------------*/