* [PATCH 0 of 3] xl: free allocations made at top level
@ 2010-07-30 9:02 Ian Campbell
2010-07-30 9:02 ` [PATCH 1 of 3] xl: return(N) from individual commands to top level instead of exit(N) Ian Campbell
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Ian Campbell @ 2010-07-30 9:02 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1 of 3] xl: return(N) from individual commands to top level instead of exit(N)
2010-07-30 9:02 [PATCH 0 of 3] xl: free allocations made at top level Ian Campbell
@ 2010-07-30 9:02 ` Ian Campbell
2010-07-30 9:02 ` [PATCH 2 of 3] xl: free the libxl context before exit Ian Campbell
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Ian Campbell @ 2010-07-30 9:02 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# 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 <ian.campbell@citrix.com>
diff -r 54981fca0742 -r 89a8086b0a05 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Fri Jul 30 08:47:39 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c Fri Jul 30 09:13:10 2010 +0100
@@ -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;
+}
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2 of 3] xl: free the libxl context before exit
2010-07-30 9:02 [PATCH 0 of 3] xl: free allocations made at top level Ian Campbell
2010-07-30 9:02 ` [PATCH 1 of 3] xl: return(N) from individual commands to top level instead of exit(N) Ian Campbell
@ 2010-07-30 9:02 ` Ian Campbell
2010-07-30 9:02 ` [PATCH 3 of 3] xl: destroy the logger " Ian Campbell
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Ian Campbell @ 2010-07-30 9:02 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280477591 -3600
# Node ID 8b2d8f18090171e161c398019bacedbb5f43455b
# Parent 89a8086b0a054c10552bc32a28026773c9608772
xl: free the libxl context before exit
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r 89a8086b0a05 -r 8b2d8f180901 tools/libxl/xl.c
--- a/tools/libxl/xl.c Fri Jul 30 09:13:10 2010 +0100
+++ b/tools/libxl/xl.c Fri Jul 30 09:13:11 2010 +0100
@@ -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;
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3 of 3] xl: destroy the logger before exit
2010-07-30 9:02 [PATCH 0 of 3] xl: free allocations made at top level Ian Campbell
2010-07-30 9:02 ` [PATCH 1 of 3] xl: return(N) from individual commands to top level instead of exit(N) Ian Campbell
2010-07-30 9:02 ` [PATCH 2 of 3] xl: free the libxl context before exit Ian Campbell
@ 2010-07-30 9:02 ` Ian Campbell
2010-07-30 9:28 ` [PATCH 0 of 3] xl: free allocations made at top level Ian Campbell
2010-07-30 14:02 ` Ian Campbell
4 siblings, 0 replies; 10+ messages in thread
From: Ian Campbell @ 2010-07-30 9:02 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280477591 -3600
# Node ID f5f5949d98f0104ad1422ddacded20875f23d38d
# Parent 8b2d8f18090171e161c398019bacedbb5f43455b
xl: destroy the logger before exit
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r 8b2d8f180901 -r f5f5949d98f0 tools/libxl/xl.c
--- a/tools/libxl/xl.c Fri Jul 30 09:13:11 2010 +0100
+++ b/tools/libxl/xl.c Fri Jul 30 09:13:11 2010 +0100
@@ -83,6 +83,7 @@ int main(int argc, char **argv)
}
libxl_ctx_free(&ctx);
+ xtl_logger_destroy((xentoollog_logger*)logger);
return ret;
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0 of 3] xl: free allocations made at top level
2010-07-30 9:02 [PATCH 0 of 3] xl: free allocations made at top level Ian Campbell
` (2 preceding siblings ...)
2010-07-30 9:02 ` [PATCH 3 of 3] xl: destroy the logger " Ian Campbell
@ 2010-07-30 9:28 ` Ian Campbell
2010-07-30 14:21 ` Ian Jackson
2010-07-30 14:02 ` Ian Campbell
4 siblings, 1 reply; 10+ messages in thread
From: Ian Campbell @ 2010-07-30 9:28 UTC (permalink / raw)
To: xen-devel@lists.xensource.com
On Fri, 2010-07-30 at 10:02 +0100, Ian Campbell wrote:
>
> 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 <ian.campbell@citrix.com>
# 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 <ian.campbell@citrix.com>
diff -r f5f5949d98f0 -r b8bf3e732b9a tools/libxc/xc_private.c
--- a/tools/libxc/xc_private.c Fri Jul 30 09:13:11 2010 +0100
+++ b/tools/libxc/xc_private.c Fri Jul 30 10:25:44 2010 +0100
@@ -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)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0 of 3] xl: free allocations made at top level
2010-07-30 9:02 [PATCH 0 of 3] xl: free allocations made at top level Ian Campbell
` (3 preceding siblings ...)
2010-07-30 9:28 ` [PATCH 0 of 3] xl: free allocations made at top level Ian Campbell
@ 2010-07-30 14:02 ` Ian Campbell
4 siblings, 0 replies; 10+ messages in thread
From: Ian Campbell @ 2010-07-30 14:02 UTC (permalink / raw)
To: xen-devel@lists.xensource.com
On Fri, 2010-07-30 at 10:02 +0100, Ian Campbell wrote:
>
> 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.
diff --git a/configure.in b/configure.in
index 62e1837..e71ecd6 100644
--- a/configure.in
+++ b/configure.in
@@ -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],
diff --git a/coregrind/Makefile.am b/coregrind/Makefile.am
index d9d1bca..d7216f9 100644
--- a/coregrind/Makefile.am
+++ b/coregrind/Makefile.am
@@ -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
diff --git a/coregrind/m_syswrap/priv_syswrap-xen.h b/coregrind/m_syswrap/priv_syswrap-xen.h
new file mode 100644
index 0000000..c65edca
--- /dev/null
+++ b/coregrind/m_syswrap/priv_syswrap-xen.h
@@ -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 ---*/
+/*--------------------------------------------------------------------*/
diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c
index 247402d..42dc4d9 100644
--- a/coregrind/m_syswrap/syswrap-linux.c
+++ b/coregrind/m_syswrap/syswrap-linux.c
@@ -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)) {
diff --git a/coregrind/m_syswrap/syswrap-xen.c b/coregrind/m_syswrap/syswrap-xen.c
new file mode 100644
index 0000000..53e6078
--- /dev/null
+++ b/coregrind/m_syswrap/syswrap-xen.c
@@ -0,0 +1,117 @@
+#include "pub_core_basics.h"
+#include "pub_core_vki.h"
+#include "pub_core_vkiscnums.h"
+#include "pub_core_threadstate.h"
+#include "pub_core_aspacemgr.h"
+#include "pub_core_debuginfo.h" // VG_(di_notify_*)
+#include "pub_core_transtab.h" // VG_(discard_translations)
+#include "pub_core_xarray.h"
+#include "pub_core_clientstate.h"
+#include "pub_core_debuglog.h"
+#include "pub_core_libcbase.h"
+#include "pub_core_libcassert.h"
+#include "pub_core_libcfile.h"
+#include "pub_core_libcprint.h"
+#include "pub_core_libcproc.h"
+#include "pub_core_libcsignal.h"
+#include "pub_core_mallocfree.h"
+#include "pub_core_tooliface.h"
+#include "pub_core_options.h"
+#include "pub_core_scheduler.h"
+#include "pub_core_signals.h"
+#include "pub_core_syscall.h"
+#include "pub_core_syswrap.h"
+
+#include "priv_types_n_macros.h"
+#include "priv_syswrap-generic.h"
+#include "priv_syswrap-xen.h"
+
+#include <stdint.h>
+
+#define __XEN_TOOLS__
+
+#include <xen/xen.h>
+#include <xen/sysctl.h>
+
+
+#define PRE(name) DEFN_PRE_TEMPLATE(xen, name)
+#define POST(name) DEFN_POST_TEMPLATE(xen, name)
+
+PRE(ioctl_privcmd_hypercall)
+{
+ struct vki_xen_privcmd_hypercall *args = (struct vki_xen_privcmd_hypercall *)(ARG3);
+
+ if (!args)
+ return;
+
+
+ switch (args->op) {
+ case __HYPERVISOR_sysctl: {
+ struct xen_sysctl *sysctl = (struct xen_sysctl *)(unsigned int)args->arg[0];
+
+ PRINT("__HYPERVISOR_sysctl ( %d )", sysctl->cmd);
+
+ /* Single argument hypercall */
+ PRE_MEM_READ("hypercall", ARG3, 8 + ( 8 * 1 ) );
+
+ /* Common part of xen_sysctl */
+ PRE_MEM_READ("__HYPERVISOR_sysctl", args->arg[0], ( 4 + 4 ));
+
+ if (!sysctl || sysctl->interface_version != XEN_SYSCTL_INTERFACE_VERSION)
+ /* BUG ? */
+ return;
+
+ //PRE_REG_READ1(long, "__HYPERVISOR_sysctl",);
+ switch (sysctl->cmd) {
+ case XEN_SYSCTL_getdomaininfolist:
+ PRE_MEM_READ("__HYPERVISOR_sysctl", &sysctl->u.getdomaininfolist.first_domain, sizeof(domid_t));
+ PRE_MEM_READ("__HYPERVISOR_sysctl", &sysctl->u.getdomaininfolist.max_domains, sizeof(uint32_t));
+ PRE_MEM_READ("__HYPERVISOR_sysctl", &sysctl->u.getdomaininfolist.buffer, sizeof(&sysctl->u.getdomaininfolist.buffer));
+ break;
+
+ default:
+ VG_(printf)("pre sysctl version %x unknown cmd %d\n",
+ sysctl->interface_version, sysctl->cmd);
+ break;
+ }
+ }
+ break;
+
+ default:
+ VG_(printf)("pre unknown hypercall %lld ( 0x%#llx, 0x%#llx, 0x%#llx, 0x%#llx, 0x%#llx )\n",
+ args->op, args->arg[0], args->arg[1], args->arg[2], args->arg[3], args->arg[4]);
+ }
+}
+
+POST(ioctl_privcmd_hypercall)
+{
+ struct vki_xen_privcmd_hypercall *args = (struct vki_xen_privcmd_hypercall *)(ARG3);
+
+ if (!args)
+ return;
+
+ switch (args->op) {
+ case __HYPERVISOR_sysctl: {
+ struct xen_sysctl *sysctl = (struct xen_sysctl *)(unsigned int)args->arg[0];
+
+ if (!sysctl || sysctl->interface_version != XEN_SYSCTL_INTERFACE_VERSION)
+ return;
+
+ switch (sysctl->cmd) {
+ case XEN_SYSCTL_getdomaininfolist:
+ POST_MEM_WRITE(&sysctl->u.getdomaininfolist.num_domains, sizeof(sysctl->u.getdomaininfolist.num_domains));
+ POST_MEM_WRITE(sysctl->u.getdomaininfolist.buffer.p, sizeof(xen_domctl_getdomaininfo_t) * sysctl->u.getdomaininfolist.num_domains);
+ break;
+ default:
+ VG_(printf)("post sysctl version %x cmd %d\n",
+ sysctl->interface_version, sysctl->cmd);
+ break;
+ }
+ break;
+ }
+ default:
+ VG_(printf)("post unknown hypercall %lld ( 0x%#llx, 0x%#llx, 0x%#llx, 0x%#llx, 0x%#llx )\n",
+ args->op, args->arg[0], args->arg[1], args->arg[2], args->arg[3], args->arg[4]);
+ break;
+ }
+}
diff --git a/include/Makefile.am b/include/Makefile.am
index 33d0857..22bffa7 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -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 \
diff --git a/include/pub_tool_vki.h b/include/pub_tool_vki.h
index 73a4174..c4c117f 100644
--- a/include/pub_tool_vki.h
+++ b/include/pub_tool_vki.h
@@ -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)
diff --git a/include/vki/vki-linux.h b/include/vki/vki-linux.h
index beff378..86126ea 100644
--- a/include/vki/vki-linux.h
+++ b/include/vki/vki-linux.h
@@ -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
/*--------------------------------------------------------------------*/
diff --git a/include/vki/vki-xen.h b/include/vki/vki-xen.h
new file mode 100644
index 0000000..7842cc9
--- /dev/null
+++ b/include/vki/vki-xen.h
@@ -0,0 +1,8 @@
+#ifndef __VKI_XEN_H
+#define __VKI_XEN_H
+
+#endif // __VKI_XEN_H
+
+/*--------------------------------------------------------------------*/
+/*--- end ---*/
+/*--------------------------------------------------------------------*/
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0 of 3] xl: free allocations made at top level
2010-07-30 9:28 ` [PATCH 0 of 3] xl: free allocations made at top level Ian Campbell
@ 2010-07-30 14:21 ` Ian Jackson
2010-07-30 14:48 ` Ian Campbell
0 siblings, 1 reply; 10+ messages in thread
From: Ian Jackson @ 2010-07-30 14:21 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel@lists.xensource.com
Ian Campbell writes ("[Xen-devel] Re: [PATCH 0 of 3] xl: free allocations made at top level"):
> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH 0 of 3] xl: free allocations made at top level
2010-07-30 14:21 ` Ian Jackson
@ 2010-07-30 14:48 ` Ian Campbell
2010-07-30 14:53 ` Ian Jackson
0 siblings, 1 reply; 10+ messages in thread
From: Ian Campbell @ 2010-07-30 14:48 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xensource.com
On Fri, 2010-07-30 at 15:21 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] Re: [PATCH 0 of 3] xl: free allocations made at top level"):
> > 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.
> 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.
> 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.
> 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.
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH 0 of 3] xl: free allocations made at top level
2010-07-30 14:48 ` Ian Campbell
@ 2010-07-30 14:53 ` Ian Jackson
2010-07-30 14:57 ` Stefano Stabellini
0 siblings, 1 reply; 10+ messages in thread
From: Ian Jackson @ 2010-07-30 14:53 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel@lists.xensource.com
Ian Campbell writes ("Re: [Xen-devel] Re: [PATCH 0 of 3] xl: free allocations made at top level"):
> 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 <ian.jackson@eu.citrix.com>
> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH 0 of 3] xl: free allocations made at top level
2010-07-30 14:53 ` Ian Jackson
@ 2010-07-30 14:57 ` Stefano Stabellini
0 siblings, 0 replies; 10+ messages in thread
From: Stefano Stabellini @ 2010-07-30 14:57 UTC (permalink / raw)
To: Ian Jackson; +Cc: Ian Campbell, xen-devel@lists.xensource.com
On Fri, 30 Jul 2010, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] Re: [PATCH 0 of 3] xl: free allocations made at top level"):
> > 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 <ian.jackson@eu.citrix.com>
I think this patch series should be applied.
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-07-30 14:57 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-07-30 9:02 [PATCH 0 of 3] xl: free allocations made at top level Ian Campbell
2010-07-30 9:02 ` [PATCH 1 of 3] xl: return(N) from individual commands to top level instead of exit(N) Ian Campbell
2010-07-30 9:02 ` [PATCH 2 of 3] xl: free the libxl context before exit Ian Campbell
2010-07-30 9:02 ` [PATCH 3 of 3] xl: destroy the logger " Ian Campbell
2010-07-30 9:28 ` [PATCH 0 of 3] xl: free allocations made at top level Ian Campbell
2010-07-30 14:21 ` Ian Jackson
2010-07-30 14:48 ` Ian Campbell
2010-07-30 14:53 ` Ian Jackson
2010-07-30 14:57 ` Stefano Stabellini
2010-07-30 14:02 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.