PR4037 and fixes to better synchronize staprun and stapio.

5 files changed, 165 insertions, 115 deletions

diff --git a/runtime/staprun/ChangeLog b/runtime/staprun/ChangeLog
index a7b00a9c..3f373863 100644
--- a/runtime/staprun/ChangeLog
+++ b/runtime/staprun/ChangeLog
@@ -1,3 +1,25 @@
+2008-01-14  Martin Hunt  <hunt@redhat.com>
+
+	PR4037 and fixes to better synchronize staprun and stapio.	
+	* symbols.c (send_data): Send header and data seperately, saving
+	a memcpy and avoiding any alignment issues.
+	(get_sections): Return -1 on error instead of just exiting
+	and leaving stapio hanging. Send data in proper format even if kernel 
+	pointers are different size.
+	(send_module): Return -1 on error instead of just exiting
+	and leaving stapio hanging.
+	(do_module): Ditto.
+	(compar): Removed.
+	(do_kernel_symbols): Rewrite to be more robust. Return -1 on 
+	error instead of just exiting and leaving stapio hanging.
+	
+	* staprun_funcs.c (handle_symbols): Signal stapio if we error out.
+	Set kernel_ptr_size;
+	
+	* staprun.h (kernel_ptr_size): Declare.
+
+	* mainloop.c (stp_main_loop): When ready, send STP_READY.
+
 2008-01-12  Frank Ch. Eigler  <fche@elastic.org>
 
 	PR 5603 horrible hack.
diff --git a/runtime/staprun/mainloop.c b/runtime/staprun/mainloop.c
index d8975d18..2e0c3c5c 100644
--- a/runtime/staprun/mainloop.c
+++ b/runtime/staprun/mainloop.c
@@ -7,7 +7,7 @@
  * Public License (GPL); either version 2, or (at your option) any
  * later version.
  *
- * Copyright (C) 2005-2007 Red Hat Inc.
+ * Copyright (C) 2005-2008 Red Hat Inc.
  */
 
 #include "staprun.h"
@@ -255,6 +255,7 @@ int stp_main_loop(void)
 	setup_main_signals(0);
 
 	dbug(2, "in main loop\n");
+	send_request(STP_READY, NULL, 0);
 
 	while (1) { /* handle messages from control channel */
 		nb = read(control_channel, recvbuf, sizeof(recvbuf));
diff --git a/runtime/staprun/staprun.h b/runtime/staprun/staprun.h
index c22cc4f3..1128fb4c 100644
--- a/runtime/staprun/staprun.h
+++ b/runtime/staprun/staprun.h
@@ -7,7 +7,7 @@
  * Public License (GPL); either version 2, or (at your option) any
  * later version.
  *
- * Copyright (C) 2005-2007 Red Hat Inc.
+ * Copyright (C) 2005-2008 Red Hat Inc.
  */
 
 #include <stdio.h>
@@ -119,7 +119,7 @@ int stp_main_loop(void);
 int send_request(int type, void *data, int len);
 void cleanup_and_exit (int);
 int do_module(void *);
-void do_kernel_symbols(void);
+int do_kernel_symbols(void);
 int init_ctl_channel(int);
 void close_ctl_channel(void);
 int init_relayfs(void);
@@ -156,6 +156,7 @@ int set_clexec(int fd);
 extern int control_channel;
 extern int ncpus;
 extern int initialized;
+extern int kernel_ptr_size;
 
 /* flags */
 extern int verbose;
diff --git a/runtime/staprun/staprun_funcs.c b/runtime/staprun/staprun_funcs.c
index 86a72985..ebf05b34 100644
--- a/runtime/staprun/staprun_funcs.c
+++ b/runtime/staprun/staprun_funcs.c
@@ -7,7 +7,7 @@
  * Public License (GPL); either version 2, or (at your option) any
  * later version.
  *
- * Copyright (C) 2007 Red Hat Inc.
+ * Copyright (C) 2007-2008 Red Hat Inc.
  */
 
 #include "staprun.h"
@@ -16,6 +16,8 @@
 #include <grp.h>
 #include <pwd.h>
 
+void cleanup(int rc);
+
 void setup_staprun_signals(void)
 {
 	struct sigaction a;
@@ -386,18 +388,20 @@ int check_permissions(void)
 }
 
 pthread_t symbol_thread_id = (pthread_t)0;
+int kernel_ptr_size = 0;
 
 /* Symbol handling thread */
 void *handle_symbols(void __attribute__((unused)) *arg)
 {
 	ssize_t nb;
 	void *data;
-	int type;
+	int32_t type;
 	char recvbuf[8192];
 
 	dbug(2, "waiting for symbol requests\n");
 
-	while (1) { /* handle messages from control channel */
+	/* handle messages from control channel */
+	while (1) {
 		nb = read(control_channel, recvbuf, sizeof(recvbuf));
 		if (nb <= 0) {
 			if (errno != EINTR)
@@ -405,14 +409,15 @@ void *handle_symbols(void __attribute__((unused)) *arg)
 			continue;
 		}
 		
-		type = *(int *)recvbuf;
-		data = (void *)(recvbuf + sizeof(int));
-
+		type = *(int32_t *)recvbuf;
+		data = (void *)(recvbuf + sizeof(int32_t));
+		
 		switch (type) { 
 		case STP_MODULE:
 		{
 			dbug(2, "STP_MODULES request received\n");
-			do_module(data);
+			if (do_module(data) < 0)
+				goto done;
 			break;
 		}		
 		case STP_SYMBOLS:
@@ -421,14 +426,15 @@ void *handle_symbols(void __attribute__((unused)) *arg)
 			dbug(2, "STP_SYMBOLS request received\n");
 			if (req->endian != 0x1234) {
 				err("ERROR: staprun is compiled with different endianess than the kernel!\n");
-				exit(1);
+				goto done;
 			}
-			if (req->ptr_size != sizeof(char *)) {
-				err("ERROR: staprun is compiled with %d-bit pointers and the kernel uses %d-bit.\n",
-					8*(int)sizeof(char *), 8*req->ptr_size);
-				exit(1);
+			kernel_ptr_size = req->ptr_size;
+			if (kernel_ptr_size != 4 && kernel_ptr_size != 8) {
+				err("ERROR: invalid kernel pointer size %d\n", kernel_ptr_size);
+				goto done;
 			}
-			do_kernel_symbols();
+			if (do_kernel_symbols() < 0)
+				goto done;
 			break;
 		}
 		default:
@@ -436,6 +442,10 @@ void *handle_symbols(void __attribute__((unused)) *arg)
 		}
 	}
 
+done:
+	/* signal stapio we're done */
+	kill(0, SIGINT);
+
 	return NULL;
 }
 
diff --git a/runtime/staprun/symbols.c b/runtime/staprun/symbols.c
index e4d96ee0..5a4855b3 100644
--- a/runtime/staprun/symbols.c
+++ b/runtime/staprun/symbols.c
@@ -1,7 +1,7 @@
 /* -*- linux-c -*-
  * Symbols and modules functions for staprun.
  *
- * Copyright (C) 2006, 2007 Red Hat Inc.
+ * Copyright (C) 2006-2008 Red Hat Inc.
  *
  * This file is part of systemtap, and is free software.  You can
  * redistribute it and/or modify it under the terms of the GNU General
@@ -10,16 +10,19 @@
  */
 
 #include "staprun.h"
-#include "../sym.h"
 
+/* send symbol data */
 static int send_data(void *data, int len)
 {
+	int32_t type = STP_SYMBOLS;
+	if (write(control_channel, &type, 4) <= 0)
+		return -1;
 	return write(control_channel, data, len);
 }
 
 /* Get the sections for a module. Put them in the supplied buffer */
 /* in the following order: */
-/* [struct _stp_module][struct _stp_symbol sections ...][string data] */
+/* [struct _stp_msg_module][struct _stp_symbol sections ...][string data]*/
 /* Return the total length of all the data. */
 
 #define SECDIR "/sys/module/%s/sections"
@@ -30,33 +33,35 @@ static int get_sections(char *name, char *data_start, int datalen)
 	char buf[32], strdata_start[32768];
 	char *strdata=strdata_start, *data=data_start;
 	int fd, len, res;
-	struct _stp_module *mod = (struct _stp_module *)data_start;
+	struct _stp_msg_module *mod = (struct _stp_msg_module *)data_start;
 	struct dirent *d;
 	DIR *secdir;
-	struct _stp_symbol *sec;
+	void *sec;
+	int struct_symbol_size = kernel_ptr_size == 8 ? sizeof(struct _stp_symbol64) : sizeof(struct _stp_symbol32);
+	uint64_t sec_addr;
 
-	/* start of data is a struct _stp_module */
-	data += sizeof(struct _stp_module);
+	/* start of data is a struct _stp_msg_module */
+	data += sizeof(struct _stp_msg_module);
 
 	res = snprintf(dir, sizeof(dir), SECDIR, name);
 	if (res >= (int)sizeof(dir)) {
 		_err("Couldn't fit module \"%s\" into dir buffer.\n"	\
 		    "This should never happen. Please file a bug report.\n", name);
-		exit(1);
+		return -1;
 	}
 	
 	if ((secdir = opendir(dir)) == NULL)
 		return 0;
 
 	/* Initialize mod. */
-	memset(mod, 0, sizeof(struct _stp_module));
+	memset(mod, 0, sizeof(struct _stp_msg_module));
 
 	/* Copy name in and check for overflow. */
 	strncpy(mod->name, name, STP_MODULE_NAME_LEN);
 	if (mod->name[STP_MODULE_NAME_LEN - 1] != '\0') {
 		_err("Couldn't fit module \"%s\" into mod->name buffer.\n" \
 		    "This should never happen. Please file a bug report.\n", name);
-		exit(1);
+		return -1;
 	}
 
 	while ((d = readdir(secdir))) {
@@ -68,7 +73,7 @@ static int get_sections(char *name, char *data_start, int datalen)
 			_err("Couldn't fit secname \"%s\" into filename buffer.\n" \
 			    "This should never happen. Please file a bug report.\n", secname);
 			closedir(secdir);
-			exit(1);
+			return -1;
 		}
 		
 		/* filter out some non-useful stuff */
@@ -88,12 +93,19 @@ static int get_sections(char *name, char *data_start, int datalen)
 		if ((fd = open(filename,O_RDONLY)) >= 0) {
 			if (read(fd, buf, 32) > 0) {
 				/* create next section */
-				sec = (struct _stp_symbol *)data;
-				if (data - data_start + (int)sizeof(struct _stp_symbol) > datalen)
+				sec = data;
+				if (data - data_start + struct_symbol_size > datalen)
 					goto err1;
-				data += sizeof(struct _stp_symbol);
-				sec->addr = strtoul(buf,NULL,16);
-				sec->symbol = (char *)(strdata - strdata_start);
+				data += struct_symbol_size;
+
+				sec_addr = (uint64_t)strtoull(buf,NULL,16);
+				if (kernel_ptr_size == 8) {
+					((struct _stp_symbol64 *)sec)->addr = sec_addr; 
+					((struct _stp_symbol64 *)sec)->symbol = (uint64_t)(strdata - strdata_start);
+				} else {
+					((struct _stp_symbol32 *)sec)->addr = (uint32_t)sec_addr;
+					((struct _stp_symbol32 *)sec)->symbol = (uint32_t)(strdata - strdata_start);
+				}
 				mod->num_sections++;
 
 				/* now create string data for the
@@ -106,13 +118,13 @@ static int get_sections(char *name, char *data_start, int datalen)
 
 				/* These sections are used a lot so keep the values handy */
 				if (!strcmp(secname, ".data") || !strncmp(secname, ".rodata", 7)) {
-					if (mod->data == 0 || sec->addr < mod->data)
-						mod->data = sec->addr;
+					if (mod->data == 0 || sec_addr < mod->data)
+						mod->data = sec_addr;
 				}
 				if (!strcmp(secname, ".text"))
-					mod->text = sec->addr;
+					mod->text = sec_addr;
 				if (!strcmp(secname, ".gnu.linkonce.this_module"))
-					mod->module = sec->addr;
+					mod->module = sec_addr;
 			}
 			close(fd);
 		}
@@ -135,56 +147,55 @@ err1:
 err0:
 	/* if this happens, something went seriously wrong. */
 	_err("Unexpected error. Overflowed buffers.\n");
-	exit(1);
-	return 0; /* not reached */
+	return -1;
 }
 #undef SECDIR
 
 /* 
  * For modules, we send the name, section names, and offsets
  */
-static void send_module (char *mname)
+static int send_module (char *mname)
 {
-	char data[32768];
-	int len = get_sections(mname, data, sizeof(data));
-	if (len) {
-		if (send_request(STP_MODULE, data, len) < 0) {
+	char data[65536];
+	int len;
+	*(int32_t *)data = STP_MODULE;
+	len = get_sections(mname, data + sizeof(int32_t),
+			   sizeof(data) - sizeof(int32_t));
+	if (len > 0) {
+		if (write(control_channel, data, len + sizeof(int32_t)) <= 0) {
 			_err("Loading of module %s failed. Exiting...\n", mname);
-			exit(1);
+			return -1;
 		}
 	}
+	return len;
 }
 
 /*
  * Send either all modules, or a specific one.
+ * Returns:
+ *   >=0 : OK
+ *    -1 : serious error (exit)
  */
 int do_module (void *data)
 {
-	struct _stp_module *mod = (struct _stp_module *)data;
+	struct _stp_msg_module *mod = (struct _stp_msg_module *)data;
 
 	if (mod->name[0] == 0) {
 		struct dirent *d;
 		DIR *moddir = opendir("/sys/module");
 		if (moddir) {
 			while ((d = readdir(moddir)))
-				send_module(d->d_name);
+				if (send_module(d->d_name) < 0) {
+					closedir(moddir);
+					return -1;
+				}
 			closedir(moddir);
 		}
-		send_request(STP_MODULE, data, 0);
-		return 1;
+		send_request(STP_MODULE, data, 1);
+		return 0;
 	}
 
-	send_module(mod->name);
-	return 0;
-}
-
-static int compar(const void *p1, const void *p2)
-{
-	struct _stp_symbol *s1 = (struct _stp_symbol *)p1;
-	struct _stp_symbol *s2 = (struct _stp_symbol *)p2;
-	if (s1->addr == s2->addr) return 0;
-	if (s1->addr < s2->addr) return -1;
-	return 1;
+	return send_module(mod->name);
 }
 
 #define MAX_SYMBOLS 32*1024
@@ -194,29 +205,29 @@ static int compar(const void *p1, const void *p2)
  * systemtap module.  Ignore module symbols; the systemtap module
  * can access them directly.
  */
-void do_kernel_symbols(void)
+int do_kernel_symbols(void)
 {
 	FILE *kallsyms=NULL;
-	char *sym_base=NULL, *data_base=NULL;
-	char buf[256], *ptr, *name, *data, *dataptr, *datamax, type;
-	unsigned long addr;
-	struct _stp_symbol *syms;
-	int num_syms, i = 0, max_syms= MAX_SYMBOLS;
-	int data_basesize = MAX_SYMBOLS*32;
-
-	sym_base = malloc(max_syms*sizeof(struct _stp_symbol)+sizeof(long));
+	char *name, *mod, *dataptr, *datamax, type, *data_base=NULL;
+	unsigned long long addr;
+	void *syms = NULL;
+	int ret, num_syms, i = 0, struct_symbol_size;
+	int max_syms= MAX_SYMBOLS, data_basesize = MAX_SYMBOLS*32;
+
+	if (kernel_ptr_size == 8) 
+		struct_symbol_size = sizeof(struct _stp_symbol64);
+	else
+		struct_symbol_size = sizeof(struct _stp_symbol32);
+
+	syms = malloc(max_syms * struct_symbol_size);
 	data_base = malloc(data_basesize);
-	if (data_base == NULL || sym_base == NULL) {
+	if (data_base == NULL || syms == NULL) {
 		_err("Failed to allocate memory for symbols\n");
 		goto err;
 	}
-	*(int *)data_base = STP_SYMBOLS;
-	dataptr = data = data_base + sizeof(long);
+	dataptr = data_base;
 	datamax = data_base + data_basesize;
 
-	*(int *)sym_base = STP_SYMBOLS;
-	syms = (struct _stp_symbol *)(sym_base + sizeof(long));
-
 	kallsyms = fopen ("/proc/kallsyms", "r");
 	if (!kallsyms) {
 		_perr("Fatal error: Unable to open /proc/kallsyms");
@@ -226,35 +237,28 @@ void do_kernel_symbols(void)
 	/* put empty string in data */
 	*dataptr++ = 0;
 
-	while (fgets_unlocked(buf, 256, kallsyms) && dataptr < datamax) {
-		addr = strtoul(buf, &ptr, 16);
-		while (isspace(*ptr)) ptr++;
-		type = *ptr++;
+	while ((ret = fscanf(kallsyms, "%llx %c %as [%as", &addr, &type, &name, &mod))>0 
+	       && dataptr < datamax) {
+		if (ret < 3)
+			continue;
+		if (ret > 3) {
+			/* ignore modules */
+			free(name);
+			free(mod);
+			/* modules are loaded above the kernel, so if we */
+			/* are getting modules, then we're done. */
+			break;
+		}
+
 		if (type == 't' || type == 'T' || type == 'A') {
-			while (isspace(*ptr)) ptr++;
-			name = ptr++;
-			while (!isspace(*ptr)) ptr++;
-			*ptr++ = 0;
-			while (*ptr && *ptr != '[') ptr++;
-			if (*ptr)
-				continue; /* it was a module */
-			syms[i].addr = addr;
-			syms[i].symbol = (char *)(dataptr - data);
-			while (*name) *dataptr++ = *name++;
-			*dataptr++ = 0;
-			i++;
-			if (i >= max_syms) {
-				char *s;
-				max_syms *= 2;
-				s = realloc(sym_base, max_syms*sizeof(struct _stp_symbol)+sizeof(long));
-				if (s == NULL) {
-					_err("Could not allocate enough space for symbols.\n");
-					goto err;
-				}
-				syms = (struct _stp_symbol *)(s + sizeof(long));
-				sym_base = s;
-			}
-			if (dataptr > datamax - 1024) {
+			if (kernel_ptr_size == 8) {
+                                ((struct _stp_symbol64 *)syms)[i].addr = (uint64_t)addr;
+                                ((struct _stp_symbol64 *)syms)[i].symbol = (uint64_t)(dataptr - data_base);
+                        } else {
+                                ((struct _stp_symbol32 *)syms)[i].addr = (uint32_t)addr;
+                                ((struct _stp_symbol32 *)syms)[i].symbol = (uint32_t)(dataptr - data_base);
+                        }
+			if (dataptr >= datamax - strlen(name)) {
 				char *db;
 				data_basesize *= 2;
 				db = realloc(data_base, data_basesize);
@@ -263,43 +267,55 @@ void do_kernel_symbols(void)
 					goto err;
 				}
 				dataptr = db + (dataptr - data_base);
-				data = db + sizeof(long);
 				datamax = db + data_basesize;
 				data_base = db;
 			}
+			strcpy(dataptr, name);
+			dataptr += strlen(name) + 1;
+			free(name);
+			i++;
+			if (i >= max_syms) {
+				max_syms *= 2;
+				syms = realloc(syms, max_syms*struct_symbol_size);
+				if (syms == NULL) {
+					_err("Could not allocate enough space for symbols.\n");
+					goto err;
+				}
+			}
 		}
 	}
 	num_syms = i;
-	qsort(syms, num_syms, sizeof(struct _stp_symbol), compar);
+	if (num_syms <= 0)
+		goto err;
 
 	/* send header */
-	*(int *)buf = STP_SYMBOLS;
-	*(int *)(buf+sizeof(long)) = num_syms;
-	*(int *)(buf+sizeof(long)+sizeof(int)) = (unsigned)(dataptr - data);
-	if (send_data(buf, 2*sizeof(int)+sizeof(long)) < 0)
+	struct _stp_msg_symbol_hdr smsh;
+	smsh.num_syms = num_syms;
+	smsh.sym_size = (uint32_t)(dataptr - data_base);
+	if (send_request(STP_SYMBOLS, &smsh, sizeof(smsh)) <= 0)
 		goto err;
 
 	/* send syms */
-	if (send_data(sym_base, num_syms*sizeof(struct _stp_symbol)+sizeof(long)) < 0)
+	if (send_data(syms, num_syms*struct_symbol_size) < 0)
 		goto err;
 	
 	/* send data */
-	if (send_data(data_base, dataptr-data+sizeof(long)) < 0)
+	if (send_data(data_base, dataptr-data_base) < 0)
 		goto err;
 
 	free(data_base);
-	free(sym_base);
+	free(syms);
 	fclose(kallsyms);
-	return;
+	return 0;
 
 err:
 	if (data_base)
 		free(data_base);
-	if (sym_base)
-		free(sym_base);
+	if (syms)
+		free(syms);
 	if (kallsyms)
 		fclose(kallsyms);
 
 	_err("Loading of symbols failed. Exiting...\n");
-	exit(1);
+	return -1;
 }