Better handling of get_activity_position() return value

sysstat · sysstat · commit 949c0a1ba93d · 2015-01-27T12:23:50.000+01:00
Function get_activity_position() can return -1 on error, ie. when
corresponding activity hasn't been found in array.
This return code was sometimes not tested because we knew that the
activity existed. So we directly used the return code as array index.
Strengthen things now: If such an activity is not found then display an
internal error message and exit.

Signed-off-by: Sebastien GODARD &lt;sysstat@users.noreply.github.com&gt;
diff --git a/common.c b/common.c
@@ -602,21 +602,19 @@ unsigned long long get_per_cpu_interval(struct stats_cpu *scc,
 
 /*
  ***************************************************************************
- * Unhandled situation: Panic and exit.
+ * Unhandled situation: Panic and exit. Should never happen.
  *
  * IN:
  * @function	Function name where situation occured.
  * @error_code	Error code.
  ***************************************************************************
  */
-#ifdef DEBUG
 void sysstat_panic(const char *function, int error_code)
 {
-	fprintf(stderr, "sysstat: %s[%d]: Last chance handler...\n",
+	fprintf(stderr, "sysstat: %s[%d]: Internal error...\n",
 		function, error_code);
 	exit(1);
 }
-#endif
 
 /*
  ***************************************************************************
diff --git a/common.h b/common.h
@@ -123,11 +123,7 @@
 
 #define MINIMUM(a,b)	((a) < (b) ? (a) : (b))
 
-#ifdef DEBUG
 #define PANIC(m)	sysstat_panic(__FUNCTION__, m)
-#else
-#define PANIC(m)
-#endif
 
 /* Number of ticks per second */
 #define HZ		hz
@@ -211,9 +207,7 @@ extern void
 	print_version(void);
 extern char *
 	strtolower(char *);
-#ifdef DEBUG
 extern void
 	sysstat_panic(const char *, int);
-#endif
 
 #endif  /* _COMMON_H */
diff --git a/sa.h b/sa.h
@@ -66,10 +66,10 @@
 
 
 /* Macro used to flag an activity that should be collected */
-#define COLLECT_ACTIVITY(m)	act[get_activity_position(act, m)]->options |= AO_COLLECTED
+#define COLLECT_ACTIVITY(m)	act[get_activity_position(act, m, EXIT_IF_NOT_FOUND)]->options |= AO_COLLECTED
 
 /* Macro used to flag an activity that should be selected */
-#define SELECT_ACTIVITY(m)	act[get_activity_position(act, m)]->options |= AO_SELECTED
+#define SELECT_ACTIVITY(m)	act[get_activity_position(act, m, EXIT_IF_NOT_FOUND)]->options |= AO_SELECTED
 
 
 /*
@@ -208,16 +208,18 @@
 #define MAX_ARGV_NR	32
 
 /* Miscellaneous constants */
-#define USE_SADC	0
-#define USE_SA_FILE	1
-#define NO_TM_START	0
-#define NO_TM_END	0
-#define NO_RESET	0
-#define NON_FATAL	0
-#define FATAL		1
-#define C_SAR		0
-#define C_SADF		1
-#define ALL_ACTIVITIES	~0U
+#define USE_SADC		0
+#define USE_SA_FILE		1
+#define NO_TM_START		0
+#define NO_TM_END		0
+#define NO_RESET		0
+#define NON_FATAL		0
+#define FATAL			1
+#define C_SAR			0
+#define C_SADF			1
+#define ALL_ACTIVITIES		~0U
+#define EXIT_IF_NOT_FOUND	1
+#define RESUME_IF_NOT_FOUND	0
 
 #define SOFT_SIZE	0
 #define HARD_SIZE	1
@@ -866,7 +868,7 @@ extern void
 extern int
 	get_activity_nr(struct activity * [], unsigned int, int);
 extern int
-	get_activity_position(struct activity * [], unsigned int);
+	get_activity_position(struct activity * [], unsigned int, int);
 extern char *
 	get_devname(unsigned int, unsigned int, int);
 extern void
diff --git a/sa_common.c b/sa_common.c
@@ -913,26 +913,28 @@ void free_bitmaps(struct activity *act[])
  * IN:
  * @act		Array of activities.
  * @act_flag	Activity flag to look for.
+ * @stop	TRUE if sysstat should exit when activity is not found.
  *
  * RETURNS:
  * Position of activity in array, or -1 if not found (this may happen when
  * reading data from a system activity file created by another version of
  * sysstat).
  ***************************************************************************
  */
-int get_activity_position(struct activity *act[], unsigned int act_flag)
+int get_activity_position(struct activity *act[], unsigned int act_flag, int stop)
 {
 	int i;
 
 	for (i = 0; i < NR_ACT; i++) {
 		if (act[i]->id == act_flag)
-			break;
+			return i;
 	}
 
-	if (i == NR_ACT)
-		return -1;
+	if (stop) {
+		PANIC((int) act_flag);
+	}
 
-	return i;
+	return -1;
 }
 
 /*
@@ -1010,7 +1012,7 @@ void select_default_activity(struct activity *act[])
 {
 	int p;
 
-	p = get_activity_position(act, A_CPU);
+	p = get_activity_position(act, A_CPU, EXIT_IF_NOT_FOUND);
 
 	/* Default is CPU activity... */
 	if (!get_activity_nr(act, AO_SELECTED, COUNT_ACTIVITIES)) {
@@ -1162,8 +1164,8 @@ void copy_structures(struct activity *act[], unsigned int id_seq[],
 		if (!id_seq[i])
 			continue;
 
-		if (((p = get_activity_position(act, id_seq[i])) < 0) ||
-		    (act[p]->nr < 1) || (act[p]->nr2 < 1)) {
+		p = get_activity_position(act, id_seq[i], EXIT_IF_NOT_FOUND);
+		if ((act[p]->nr < 1) || (act[p]->nr2 < 1)) {
 			PANIC(1);
 		}
 
@@ -1193,7 +1195,7 @@ void read_file_stat_bunch(struct activity *act[], int curr, int ifd, int act_nr,
 
 	for (i = 0; i < act_nr; i++, fal++) {
 
-		if (((p = get_activity_position(act, fal->id)) < 0) ||
+		if (((p = get_activity_position(act, fal->id, RESUME_IF_NOT_FOUND)) < 0) ||
 		    (act[p]->magic != fal->magic)) {
 			/*
 			 * Ignore current activity in file, which is unknown to
@@ -1221,7 +1223,7 @@ void read_file_stat_bunch(struct activity *act[], int curr, int ifd, int act_nr,
 			sa_fread(ifd, act[p]->buf[curr], act[p]->fsize * act[p]->nr * act[p]->nr2, HARD_SIZE);
 		}
 		else {
-			PANIC(act[p]->nr);
+			PANIC(p);
 		}
 	}
 }
@@ -1337,7 +1339,7 @@ void check_file_actlst(int *ifd, char *dfile, struct activity *act[],
 			handle_invalid_sa_file(ifd, file_magic, dfile, 0);
 		}
 
-		if ((p = get_activity_position(act, fal->id)) < 0)
+		if ((p = get_activity_position(act, fal->id, RESUME_IF_NOT_FOUND)) < 0)
 			/* Unknown activity */
 			continue;
 
@@ -1440,7 +1442,7 @@ int reallocate_vol_act_structures(struct activity *act[], unsigned int act_nr,
 {
 	int j, p;
 
-	if ((p = get_activity_position(act, act_id)) < 0)
+	if ((p = get_activity_position(act, act_id, RESUME_IF_NOT_FOUND)) < 0)
 		/* Ignore unknown activity */
 		return -1;
 
@@ -1538,15 +1540,15 @@ int parse_sar_opt(char *argv[], int *opt, struct activity *act[],
 
 			/* Force '-P ALL -I XALL -r ALL -u ALL' */
 
-			p = get_activity_position(act, A_MEMORY);
+			p = get_activity_position(act, A_MEMORY, EXIT_IF_NOT_FOUND);
 			act[p]->opt_flags |= AO_F_MEM_AMT + AO_F_MEM_DIA +
 					     AO_F_MEM_SWAP + AO_F_MEM_ALL;
 
-			p = get_activity_position(act, A_IRQ);
+			p = get_activity_position(act, A_IRQ, EXIT_IF_NOT_FOUND);
 			set_bitmap(act[p]->bitmap->b_array, ~0,
 				   BITMAP_SIZE(act[p]->bitmap->b_size));
 
-			p = get_activity_position(act, A_CPU);
+			p = get_activity_position(act, A_CPU, EXIT_IF_NOT_FOUND);
 			set_bitmap(act[p]->bitmap->b_array, ~0,
 				   BITMAP_SIZE(act[p]->bitmap->b_size));
 			act[p]->opt_flags = AO_F_CPU_ALL;
@@ -1573,7 +1575,7 @@ int parse_sar_opt(char *argv[], int *opt, struct activity *act[],
 			break;
 
 		case 'H':
-			p = get_activity_position(act, A_HUGE);
+			p = get_activity_position(act, A_HUGE, EXIT_IF_NOT_FOUND);
 			act[p]->options   |= AO_SELECTED;
 			break;
 
@@ -1611,7 +1613,7 @@ int parse_sar_opt(char *argv[], int *opt, struct activity *act[],
 			break;
 
 		case 'r':
-			p = get_activity_position(act, A_MEMORY);
+			p = get_activity_position(act, A_MEMORY, EXIT_IF_NOT_FOUND);
 			act[p]->options   |= AO_SELECTED;
 			act[p]->opt_flags |= AO_F_MEM_AMT;
 			if (!*(argv[*opt] + i + 1) && argv[*opt + 1] && !strcmp(argv[*opt + 1], K_ALL)) {
@@ -1622,13 +1624,13 @@ int parse_sar_opt(char *argv[], int *opt, struct activity *act[],
 			break;
 
 		case 'R':
-			p = get_activity_position(act, A_MEMORY);
+			p = get_activity_position(act, A_MEMORY, EXIT_IF_NOT_FOUND);
 			act[p]->options   |= AO_SELECTED;
 			act[p]->opt_flags |= AO_F_MEM_DIA;
 			break;
 
 		case 'S':
-			p = get_activity_position(act, A_MEMORY);
+			p = get_activity_position(act, A_MEMORY, EXIT_IF_NOT_FOUND);
 			act[p]->options   |= AO_SELECTED;
 			act[p]->opt_flags |= AO_F_MEM_SWAP;
 			break;
@@ -1648,7 +1650,7 @@ int parse_sar_opt(char *argv[], int *opt, struct activity *act[],
 			break;
 
 		case 'u':
-			p = get_activity_position(act, A_CPU);
+			p = get_activity_position(act, A_CPU, EXIT_IF_NOT_FOUND);
 			act[p]->options |= AO_SELECTED;
 			if (!*(argv[*opt] + i + 1) && argv[*opt + 1] && !strcmp(argv[*opt + 1], K_ALL)) {
 				(*opt)++;
@@ -1866,7 +1868,7 @@ int parse_sar_I_opt(char *argv[], int *opt, struct activity *act[])
 	char *t;
 
 	/* Select interrupt activity */
-	p = get_activity_position(act, A_IRQ);
+	p = get_activity_position(act, A_IRQ, EXIT_IF_NOT_FOUND);
 	act[p]->options |= AO_SELECTED;
 
 	for (t = strtok(argv[*opt], ","); t; t = strtok(NULL, ",")) {
@@ -1924,7 +1926,7 @@ int parse_sa_P_opt(char *argv[], int *opt, unsigned int *flags, struct activity
 	int i, p;
 	char *t;
 
-	p = get_activity_position(act, A_CPU);
+	p = get_activity_position(act, A_CPU, EXIT_IF_NOT_FOUND);
 
 	if (argv[++(*opt)]) {
 
diff --git a/sa_conv.c b/sa_conv.c
@@ -488,7 +488,7 @@ int upgrade_header_section(char dfile[], int fd, int stdfd,
 			return -1;
 		}
 
-		if ((p = get_activity_position(act, fal->id)) >= 0) {
+		if ((p = get_activity_position(act, fal->id, RESUME_IF_NOT_FOUND)) >= 0) {
 			/* This is a known activity, maybe with an unknown format */
 
 			if (IS_VOLATILE(act[p]->options) && previous_format) {
@@ -597,7 +597,7 @@ int upgrade_activity_section(int stdfd, struct activity *act[],
 	for (i = 0; i < file_hdr->sa_act_nr; i++, fal++) {
 
 		file_act = *fal;
-		if ((p = get_activity_position(act, fal->id)) >= 0) {
+		if ((p = get_activity_position(act, fal->id, RESUME_IF_NOT_FOUND)) >= 0) {
 			/* Update activity magic number */
 			file_act.magic = act[p]->magic;
 			/* Also update its size, which may have changed with recent versions */
@@ -693,7 +693,7 @@ int upgrade_restart_record(int fd, int stdfd, struct activity *act[],
 			memset(&file_act, 0, FILE_ACTIVITY_SIZE);
 
 			/* Old format: Sequence of volatile activities is in vol_id_seq */
-			p = get_activity_position(act, vol_id_seq[i]);
+			p = get_activity_position(act, vol_id_seq[i], EXIT_IF_NOT_FOUND);
 
 			/* Set only the necessary fields */
 			file_act.id = act[p]->id;
@@ -742,7 +742,7 @@ int upgrade_common_record(int fd, int stdfd, struct activity *act[],
 	fal = file_actlst;
 	for (i = 0; i < file_hdr->sa_act_nr; i++, fal++) {
 
-		if ((p = get_activity_position(act, fal->id)) < 0) {
+		if ((p = get_activity_position(act, fal->id, RESUME_IF_NOT_FOUND)) < 0) {
 			/* An unknown activity should still be read and written */
 			size = (size_t) fal->size * (size_t) fal->nr * (size_t) fal->nr2;
 			SREALLOC(buffer, void, size);
diff --git a/sadc.c b/sadc.c
@@ -506,7 +506,7 @@ void setup_file_hdr(int fd)
 	 * A_CPU activity is always collected, hence its number of items is
 	 * always counted (in sa_sys_init()).
 	 */
-	file_hdr.sa_last_cpu_nr = act[get_activity_position(act, A_CPU)]->nr;
+	file_hdr.sa_last_cpu_nr = act[get_activity_position(act, A_CPU, EXIT_IF_NOT_FOUND)]->nr;
 
 	/* Get system name, release number, hostname and machine architecture */
 	uname(&header);
@@ -532,7 +532,7 @@ void setup_file_hdr(int fd)
 		 */
 		if (!id_seq[i])
 			continue;
-		if ((p = get_activity_position(act, id_seq[i])) < 0)
+		if ((p = get_activity_position(act, id_seq[i], RESUME_IF_NOT_FOUND)) < 0)
 			continue;
 
 		if (IS_COLLECTED(act[p]->options)) {
@@ -592,7 +592,7 @@ void write_vol_act_structures(int ofd)
 			file_act.id = file_act.nr = 0;
 		}
 		else {
-			p = get_activity_position(act, vol_id_seq[i]);
+			p = get_activity_position(act, vol_id_seq[i], EXIT_IF_NOT_FOUND);
 
 			/*
 			 * All the fields in file_activity structure are not used.
@@ -692,7 +692,7 @@ void write_stats(int ofd)
 
 		if (!id_seq[i])
 			continue;
-		if ((p = get_activity_position(act, id_seq[i])) < 0)
+		if ((p = get_activity_position(act, id_seq[i], RESUME_IF_NOT_FOUND)) < 0)
 			continue;
 
 		if (IS_COLLECTED(act[p]->options)) {
@@ -910,7 +910,7 @@ void open_ofile(int *ofd, char ofile[], int restart_mark)
 				handle_invalid_sa_file(ofd, &file_magic, ofile, 0);
 			}
 
-			p = get_activity_position(act, file_act[i].id);
+			p = get_activity_position(act, file_act[i].id, RESUME_IF_NOT_FOUND);
 
 			if ((p < 0) || (act[p]->fsize != file_act[i].size) ||
 			    (act[p]->magic != file_act[i].magic))
@@ -940,7 +940,7 @@ void open_ofile(int *ofd, char ofile[], int restart_mark)
 
 		for (i = 0; i < file_hdr.sa_act_nr; i++) {
 
-			p = get_activity_position(act, file_act[i].id);
+			p = get_activity_position(act, file_act[i].id, EXIT_IF_NOT_FOUND);
 
 			/*
 			 * Force number of items (serial lines, network interfaces...)
@@ -970,7 +970,7 @@ void open_ofile(int *ofd, char ofile[], int restart_mark)
 			vol_id_seq[j++] = 0;
 		}
 
-		p = get_activity_position(act, A_CPU);
+		p = get_activity_position(act, A_CPU, EXIT_IF_NOT_FOUND);
 		if (!IS_COLLECTED(act[p]->options)) {
 			/* A_CPU activity should always exist in file */
 			goto append_error;
@@ -1020,7 +1020,7 @@ void open_ofile(int *ofd, char ofile[], int restart_mark)
 void read_stats(void)
 {
 	int i;
-	__nr_t cpu_nr = act[get_activity_position(act, A_CPU)]->nr;
+	__nr_t cpu_nr = act[get_activity_position(act, A_CPU, EXIT_IF_NOT_FOUND)]->nr;
 
 	/*
 	 * Init uptime0. So if /proc/uptime cannot fill it,
diff --git a/sadf.c b/sadf.c
@@ -1378,10 +1378,7 @@ void main_display_loop(int ifd, struct file_activity *file_actlst, __nr_t cpu_nr
 				if (!id_seq[i])
 					continue;
 
-				if ((p = get_activity_position(act, id_seq[i])) < 0) {
-					/* Should never happen */
-					PANIC(1);
-				}
+				p = get_activity_position(act, id_seq[i], EXIT_IF_NOT_FOUND);
 				if (!IS_SELECTED(act[p]->options))
 					continue;
 
@@ -1466,7 +1463,7 @@ void read_stats_from_file(char dfile[])
 			  &file_actlst, id_seq, ignore);
 
 	/* Now pick up number of proc for this file */
-	cpu_nr = act[get_activity_position(act, A_CPU)]->nr;
+	cpu_nr = act[get_activity_position(act, A_CPU, EXIT_IF_NOT_FOUND)]->nr;
 
 	if (DISPLAY_HDR_ONLY(flags)) {
 		if (*fmt[f_position]->f_header) {
diff --git a/sadf_misc.c b/sadf_misc.c
diff --git a/sar.c b/sar.c
