powerpc-utils-devel

[Powerpc-utils-devel] [PATCH] Cleanup build warnings

[Nathan F. <nf...@li...>]

Code cleanup to get rid of warnings generated when building powerpc-utils.

For the most part this involves simple code changes. For the serv_config
the code updates needed to get rid of the strict-aliasing warnings I thought
to be more work than was worth the effort so I simply disabled strict-aliasing
for this command.

Signed-off-by: Nathan Fontenot <nf...@li...>
---
 Makefile.am                 |    1 +
 src/drmgr/common.c          |   20 +++++++++++++-------
 src/drmgr/common_cpu.c      |    6 +++++-
 src/drmgr/common_pci.c      |    7 ++++++-
 src/drmgr/drmig_chrp_pmig.c |    5 +++--
 src/drmgr/drslot_chrp_cpu.c |   27 ++++++++++++++++-----------
 src/drmgr/drslot_chrp_mem.c |   11 ++++++++---
 src/lparstat.c              |   40 +++++++++++++++++++++++++++-------------
 src/nvram.c                 |    7 +++++--
 src/ppc64_cpu.c             |   20 +++++++++++---------
 src/rtas_ibm_get_vpd.c      |    3 +--
 src/serv_config.c           |   17 ++++++++---------
 12 files changed, 104 insertions(+), 60 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index aa2befb..9a00d92 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -93,6 +93,7 @@ src_rtas_ibm_get_vpd_SOURCES = src/rtas_ibm_get_vpd.c $(librtas_error_SOURCES) $
 src_rtas_ibm_get_vpd_LDADD = -lrtas
 
 src_serv_config_SOURCES = src/serv_config.c $(librtas_error_SOURCES) $(pseries_platform_SOURCES)
+src_serv_config_CFLAGS = $(AM_CFLAGS) -fno-strict-aliasing
 src_serv_config_LDADD = -lrtas
 
 src_uesensor_SOURCES = src/uesensor.c $(librtas_error_SOURCES) $(pseries_platform_SOURCES)
diff --git a/src/drmgr/common.c b/src/drmgr/common.c
index 0cb621d..0452168 100644
--- a/src/drmgr/common.c
+++ b/src/drmgr/common.c
@@ -53,7 +53,7 @@ int say(enum say_level lvl, char *fmt, ...)
 {
 	va_list ap;
 	char buf[256];
-	int len, rc;
+	int len;
 
 	va_start(ap, fmt);
 	memset(buf, 0, 256);
@@ -66,7 +66,7 @@ int say(enum say_level lvl, char *fmt, ...)
 	}
 
 	if (log_fd)
-		rc = write(log_fd, buf, len);
+		len = write(log_fd, buf, len);
 
 	if (lvl <= output_level)
 		fprintf(stderr, "%s", buf);
@@ -627,7 +627,7 @@ get_att_prop(const char *path, const char *name, char *buf, size_t buf_sz,
 	     const char *attr_type)
 {
 	FILE *fp;
-	int rc;
+	int rc = 0;
 	char dir[DR_PATH_MAX];
 
 	if (buf == NULL)
@@ -648,17 +648,23 @@ get_att_prop(const char *path, const char *name, char *buf, size_t buf_sz,
 	 * either /proc or sysfs so it works and is cheaper than a strcmp()
 	 */
 	switch (dir[1]) {
-	    case 'p':	/* /proc */
-		rc = fread(buf, buf_sz, 1, fp);
+	case 'p':	/* /proc */
+		if (fread(buf, buf_sz, 1, fp) != 1)
+			rc = -1;
 		break;
 
-	    case 's':	/* sysfs */
+	case 's':	/* sysfs */
 		rc = fscanf(fp, attr_type, (int *)buf);
+		if (rc == EOF)
+			rc = ferror(fp);
 		break;
+
+	default:
+		rc = -1;
 	}
 
 	fclose(fp);
-	return 0;
+	return rc;
 }
 
 /**
diff --git a/src/drmgr/common_cpu.c b/src/drmgr/common_cpu.c
index e4c8082..c36f669 100644
--- a/src/drmgr/common_cpu.c
+++ b/src/drmgr/common_cpu.c
@@ -194,8 +194,12 @@ cpu_index_to_path(struct dr_node *cpu)
 
 	closedir(d);
 
-	if (found)
+	if (found) {
 		snprintf(cpu->ofdt_path, DR_PATH_MAX, "%s", path);
+		rc = 0;
+	} else {
+		rc = -1;
+	}
 
 	return rc;
 }
diff --git a/src/drmgr/common_pci.c b/src/drmgr/common_pci.c
index 04a8a35..d646a94 100644
--- a/src/drmgr/common_pci.c
+++ b/src/drmgr/common_pci.c
@@ -1037,7 +1037,12 @@ get_bus_id(char *loc_code)
 		if (f == NULL)
 			continue;
 
-		fread(location, sizeof(location), 1, f);
+		if (fread(location, sizeof(location), 1, f) != 1) {
+			say(ERROR, "Could not read phys_location\n");
+			fclose(f);
+			return NULL;
+		}
+
 		fclose(f);
 
 		/* Strip any newline from the location to compare */
diff --git a/src/drmgr/drmig_chrp_pmig.c b/src/drmgr/drmig_chrp_pmig.c
index 8e8f9d0..68b20b8 100644
--- a/src/drmgr/drmig_chrp_pmig.c
+++ b/src/drmgr/drmig_chrp_pmig.c
@@ -15,6 +15,7 @@
 #include <sys/types.h>
 #include <dirent.h>
 #include <time.h>
+#include <inttypes.h>
 #include <librtas.h>
 #include "dr.h"
 #include "ofdt.h"
@@ -582,7 +583,7 @@ int do_migration(uint64_t stream_val)
 		rc = rtas_suspend_me(stream_val);
 		say(DEBUG, "ibm,suspend-me() returned %d\n", rc);
 	} else if (api_level == MIGRATION_API_V1) {
-		sprintf(buf, "0x%llx\n", stream_val);
+		sprintf(buf, "0x%" PRIx64 "\n", stream_val);
 
 		fd = open(SYSFS_MIGRATION_FILE, O_WRONLY);
 		if (fd == -1) {
@@ -616,7 +617,7 @@ int do_hibernation(uint64_t stream_val)
 	int rc, fd;
 	char buf[64];
 
-	sprintf(buf, "0x%llx\n", stream_val);
+	sprintf(buf, "0x%" PRIx64 "\n", stream_val);
 
 	fd = open(SYSFS_HIBERNATION_FILE, O_WRONLY);
 	if (fd == -1) {
diff --git a/src/drmgr/drslot_chrp_cpu.c b/src/drmgr/drslot_chrp_cpu.c
index 54fc576..f34854c 100644
--- a/src/drmgr/drslot_chrp_cpu.c
+++ b/src/drmgr/drslot_chrp_cpu.c
@@ -352,8 +352,8 @@ int
 drslot_chrp_cpu(struct options *opts)
 {
 	struct dr_info dr_info;
-	int rc = -1;
-
+	int rc;
+	
 	if (! cpu_dlpar_capable()) {
 		say(ERROR, "CPU DLPAR capability is not enabled on this "
 		    "platform.\n");
@@ -386,15 +386,20 @@ drslot_chrp_cpu(struct options *opts)
 
 	if (opts->p_option && (strcmp(opts->p_option, "smt_threads") == 0)) {
 		rc = smt_threads_func(opts, &dr_info);
-	} else {
-		switch (opts->action) {
-		    case ADD:
-			rc = add_cpus(opts, &dr_info);
-			break;
-		    case REMOVE:
-			rc = remove_cpus(opts, &dr_info);
-			break;
-		}
+		free_cpu_drc_info(&dr_info);
+		return rc;
+	} 
+	
+	switch (opts->action) {
+	case ADD:
+		rc = add_cpus(opts, &dr_info);
+		break;
+	case REMOVE:
+		rc = remove_cpus(opts, &dr_info);
+		break;
+	default:
+		rc = -1;
+		break;
 	}
 
 	free_cpu_drc_info(&dr_info);
diff --git a/src/drmgr/drslot_chrp_mem.c b/src/drmgr/drslot_chrp_mem.c
index ce58c42..dc4c50c 100644
--- a/src/drmgr/drslot_chrp_mem.c
+++ b/src/drmgr/drslot_chrp_mem.c
@@ -671,7 +671,7 @@ update_drconf_node(struct dr_node *lmb, struct lmb_list_head *lmb_list,
 	memcpy(tmp, lmb_list->drconf_buf, lmb_list->drconf_buf_sz);
 	tmp += lmb_list->drconf_buf_sz;
 
-	tmp += sprintf(tmp, " %s %d ", (action == ADD ? "add" : "remove"),
+	tmp += sprintf(tmp, " %s %ld ", (action == ADD ? "add" : "remove"),
 		       sizeof(lmb->lmb_address));
 	memcpy(tmp, &lmb->lmb_address, sizeof(lmb->lmb_address));
 	tmp += sizeof(lmb->lmb_address);
@@ -829,7 +829,6 @@ set_mem_scn_state(struct mem_scn *mem_scn, int state)
 	int file;
 	char path[DR_PATH_MAX];
 	int rc = 0;
-	int unused;
 	time_t t;
 	char tbuf[128];
 
@@ -848,9 +847,15 @@ set_mem_scn_state(struct mem_scn *mem_scn, int state)
 		return -1;
 	}
 
-	unused = write(file, state_strs[state], strlen(state_strs[state]));
+	rc = write(file, state_strs[state], strlen(state_strs[state]));
 	close(file);
 
+	if (rc == -1) {
+		say(ERROR, "Failed to mark %s %s\n", mem_scn->sysfs_path,
+		    state_strs[state]);
+		return rc;
+	}
+
 	if (get_mem_scn_state(mem_scn) != state) {
 		time(&t);
 		strftime(tbuf, 128, "%T", localtime(&t));
diff --git a/src/lparstat.c b/src/lparstat.c
index 8c4c71d..2100075 100644
--- a/src/lparstat.c
+++ b/src/lparstat.c
@@ -140,7 +140,7 @@ int parse_lparcfg()
 {
 	FILE *f;
 	char line[128];
-	char *unused;
+	int first_line = 1;
 
 	f = fopen(LPARCFG_FILE, "r");
 	if (!f) {
@@ -149,11 +149,15 @@ int parse_lparcfg()
 	}
 
 	/* parse the file skipping the first line */
-	unused = fgets(line, 128, f);
 	while (fgets(line, 128, f) != NULL) {
 		char *name, *value, *nl;
 		struct sysentry *se;
 
+		if (first_line) {
+			first_line = 0;
+			continue;
+		}
+
 		if (line[0] == '\n')
 			continue;
 
@@ -211,13 +215,16 @@ int parse_proc_stat()
 	int i, entries = 6;
 	long long statvals[entries];
 	struct sysentry *se;
-	char *unused;
 	char *names[] = {"cpu_total", "cpu_user", "cpu_nice", "cpu_sys",
 			 "cpu_idle", "cpu_iowait"};
 
 	/* we just need the first line */
 	f = fopen("/proc/stat", "r");
-	unused = fgets(line, 128, f);
+	if (fgets(line, 128, f) == NULL) {
+		fclose(f);
+		return -1;
+	}
+
 	fclose(f);
 
 	statvals[0] = 0;
@@ -312,17 +319,17 @@ void get_name(const char *file, char *buf)
 {
 	FILE *f;
 	char tmpbuf[64];
-	int rc;
 
 	f = fopen(file, "r");
-	if(!f) {
+	if (!f) {
 		sprintf(buf, "%c", '\0');
 		return;
 	}
-	rc = fread(tmpbuf, 64, 1, f);
-	fclose(f);
 
-	sprintf(buf, "%s", tmpbuf);
+	if (fread(tmpbuf, 64, 1, f) == 1)
+		sprintf(buf, "%s", tmpbuf);
+
+	fclose(f);
 }
 
 void get_node_name(struct sysentry *se, char *buf)
@@ -347,10 +354,14 @@ void get_mem_total(struct sysentry *se, char *buf)
 {
 	FILE *f;
 	char line[128];
-	char *mem, *nl, *unused;
+	char *mem, *nl;
 
 	f = fopen("/proc/meminfo", "r");
-	unused = fgets(line, 128, f);
+	if (fgets(line, 128, f) == NULL) {
+		fclose(f);
+		return;
+	}
+
 	fclose(f);
 
 	mem = strchr(line, ':');
@@ -369,10 +380,13 @@ void get_smt_mode(struct sysentry *se, char *buf)
 	FILE *f;
 	char line[128];
 	char *cmd = "/usr/sbin/ppc64_cpu --smt";
-	char *unused;
 
 	f = popen(cmd, "r");
-	unused = fgets(line, 128, f);
+	if (fgets(line, 128, f) == NULL) {
+		pclose(f);
+		return;
+	}
+
 	pclose(f);
 
 	/* The output is either "SMT=x" or "SMT is off", we can cheat
diff --git a/src/nvram.c b/src/nvram.c
index 4d968d0..09c9887 100644
--- a/src/nvram.c
+++ b/src/nvram.c
@@ -769,7 +769,7 @@ dump_errlog(struct nvram *nvram)
     uint16_t *sys_regs;	    /* System specific registers 
                              * (e.g. bus arbitration chips, etc */
     uint16_t *cpu_regs[MAX_CPUS+1];
-    uint16_t *memctrl_data;
+    /* uint16_t *memctrl_data; << See memctrl_data section below */
     uint16_t *ioctrl_data;
 
     phead = nvram_find_partition(nvram, NVRAM_SIG_SP, "ibm,err-log", NULL);
@@ -821,11 +821,14 @@ dump_errlog(struct nvram *nvram)
     num_memctrls = ntohs(p[i]);
     printf("Memory Controllers: %d\n", num_memctrls);
 
-    /* next index is offset of memory controller data */
+    /* next index is offset of memory controller data, this is not used
+       so just skip over it. */
     i++;	/* ToDo: this may be a list of offsets...manual doesn't show 
 		   that but only 1 seems odd */
+#if 0
     offset = ntohs(p[i])/2+1;
     memctrl_data = offset + i < p_max ? p + offset + i : 0;
+#endif
 
     /* next index is number of I/O Subsystem controllers */
     i++;
diff --git a/src/ppc64_cpu.c b/src/ppc64_cpu.c
index 42994ca..46aed33 100644
--- a/src/ppc64_cpu.c
+++ b/src/ppc64_cpu.c
@@ -694,7 +694,10 @@ static int do_smt_snooze_delay(char *state)
 
 static int do_run_mode(char *run_mode)
 {
-	char mode[3];
+	struct {
+		uint16_t	len;
+		uint8_t		val;
+	} rmode;
 	int rc;
 
 	if (getuid() != 0) {
@@ -704,7 +707,7 @@ static int do_run_mode(char *run_mode)
 	}
 
 	if (!run_mode) {
-		rc = rtas_get_sysparm(DIAGNOSTICS_RUN_MODE, 3, mode);
+		rc = rtas_get_sysparm(DIAGNOSTICS_RUN_MODE, 3, (char *)&rmode);
 		if (rc) {
 			if (rc == -3) {
 				printf("Machine does not support diagnostic "
@@ -720,19 +723,18 @@ static int do_run_mode(char *run_mode)
 				       "mode\n");
 			}
 		} else
-			printf("run-mode=%d\n", mode[2]);
+			printf("run-mode=%d\n", rmode.val);
 	} else {
-		short rmode = atoi(run_mode);
+		rmode.val = atoi(run_mode);
 
-		if (rmode < 0 || rmode > 3) {
-			printf("Invalid run-mode=%d\n", rmode);
+		if (rmode.val < 0 || rmode.val > 3) {
+			printf("Invalid run-mode=%d\n", rmode.val);
 			return -1;
 		}
 
-		*(short *)mode = htobe16(1);
-		mode[2] = rmode;
+		rmode.len = 1;
 
-		rc = rtas_set_sysparm(DIAGNOSTICS_RUN_MODE, mode);
+		rc = rtas_set_sysparm(DIAGNOSTICS_RUN_MODE, (char *)&rmode);
 		if (rc) {
 			if (rc == -3) {
 				printf("Machine does not support diagnostic "
diff --git a/src/rtas_ibm_get_vpd.c b/src/rtas_ibm_get_vpd.c
index b2d94ce..c2ffd9a 100644
--- a/src/rtas_ibm_get_vpd.c
+++ b/src/rtas_ibm_get_vpd.c
@@ -111,7 +111,7 @@ int main(int argc, char **argv)
 {
 	char *loc_code = "";
 	char err_buf[ERR_BUF_SIZE];
-	int lflag = 0, rc, c;
+	int rc, c;
 	unsigned int seq = 1, next_seq;
 	struct buf_element *list, *current;
 
@@ -133,7 +133,6 @@ int main(int argc, char **argv)
 		switch (c) {
 		case 'l':
 			loc_code = optarg;
-			lflag = 1;
 			break;
 		case 'h':
 			print_help(argv[0]);
diff --git a/src/serv_config.c b/src/serv_config.c
index 46e02f3..900f0f3 100644
--- a/src/serv_config.c
+++ b/src/serv_config.c
@@ -463,7 +463,7 @@ int
 update_nvram(char *var, char *val, char *partition) {
 	char buf[256];
 	pid_t child;
-	int status, rc;
+	int status;
 	char *nvram_args[] = { "nvram", "--update-config",
 			buf, "-p", partition, NULL };
 
@@ -482,12 +482,12 @@ update_nvram(char *var, char *val, char *partition) {
 	}
 	else if (child == 0) {
 		/* child process */
-		rc = execv(NVRAM_PROGRAM, nvram_args);
-
-		/* shouldn't get here */
-		err_msg(ERR_MSG, "Could not exec %s to update NVRAM\n",
+		if (execv(NVRAM_PROGRAM, nvram_args)) {
+			/* shouldn't get here */
+			err_msg(ERR_MSG, "Could not exec %s to update NVRAM\n",
 				NVRAM_PROGRAM);
-		exit(1);
+			exit(1);
+		}
 	}
 	else {
 		/* parent process */
@@ -609,12 +609,11 @@ byte_to_string(uint8_t num, char *buf, size_t size) {
  */
 int
 parse_call_home_buffer(char *var, char *buf, size_t size) {
-	int buf_size;
 	char *loc;
 
 	if (!call_home_buffer) return RC_OTHER;	/* should never happen */
 
-	buf_size = be16toh(*(uint16_t *)call_home_buffer);
+	/* The first 16 bits is the call home bufer size, skip past this. */
 	loc = call_home_buffer + sizeof(uint16_t);
 
 	while (loc[0] != '\0') {
@@ -674,7 +673,7 @@ retrieve_value(struct service_var *var, char *buf, size_t size) {
 			if (var->sysparm_num == USE_CALL_HOME_SYSPARM)
 				break;
 
-			ret_size = be16toh(*(uint16_t *)param);
+			ret_size = be16toh(*param);
 			if (!strcmp(var->nvram_var, "sp-ri-pon") ||
 				!strcmp(var->nvram_var, "sp-remote-pon") ||
 				!strcmp(var->nvram_var, "sp-sen")) {

Re: [Powerpc-utils-devel] [PATCH] Cleanup build warnings

[Kamalesh B. <kam...@li...>]

On 06/30/2015 10:42 PM, Nathan Fontenot wrote:
> Code cleanup to get rid of warnings generated when building powerpc-utils.
>
> For the most part this involves simple code changes. For the serv_config
> the code updates needed to get rid of the strict-aliasing warnings I thought
> to be more work than was worth the effort so I simply disabled strict-aliasing
> for this command.
>
> Signed-off-by: Nathan Fontenot <nf...@li...>
> ---
[...]

This patch fixes the lparstat.c. 'unsued' variable warning.

>  /**
> diff --git a/src/drmgr/common_cpu.c b/src/drmgr/common_cpu.c
> index e4c8082..c36f669 100644
> --- a/src/drmgr/common_cpu.c
> +++ b/src/drmgr/common_cpu.c
> @@ -194,8 +194,12 @@ cpu_index_to_path(struct dr_node *cpu)
>
>  	closedir(d);
>
> -	if (found)
> +	if (found) {
>  		snprintf(cpu->ofdt_path, DR_PATH_MAX, "%s", path);
> +		rc = 0;
> +	} else {
> +		rc = -1;
> +	}

We can avoid this else block by declaring  = -1;

>  	return rc;
>  }
[...]

> diff --git a/src/drmgr/drslot_chrp_cpu.c b/src/drmgr/drslot_chrp_cpu.c
> index 54fc576..f34854c 100644
> --- a/src/drmgr/drslot_chrp_cpu.c
> +++ b/src/drmgr/drslot_chrp_cpu.c
> @@ -352,8 +352,8 @@ int
>  drslot_chrp_cpu(struct options *opts)
>  {
>  	struct dr_info dr_info;
> -	int rc = -1;
> -
> +	int rc;
> +	

Above line introduces trailing white spaces.

>  	if (! cpu_dlpar_capable()) {
>  		say(ERROR, "CPU DLPAR capability is not enabled on this "
>  		    "platform.\n");
> @@ -386,15 +386,20 @@ drslot_chrp_cpu(struct options *opts)
>
>  	if (opts->p_option && (strcmp(opts->p_option, "smt_threads") == 0)) {
>  		rc = smt_threads_func(opts, &dr_info);
> -	} else {
> -		switch (opts->action) {
> -		    case ADD:
> -			rc = add_cpus(opts, &dr_info);
> -			break;
> -		    case REMOVE:
> -			rc = remove_cpus(opts, &dr_info);
> -			break;
> -		}
> +		free_cpu_drc_info(&dr_info);
> +		return rc;
> +	} 
> +	

Above line introduces trailing white spaces.
> +	switch (opts->action) {
> +	case ADD:
> +		rc = add_cpus(opts, &dr_info);
> +		break;
> +	case REMOVE:
> +		rc = remove_cpus(opts, &dr_info);
> +		break;
> +	default:
> +		rc = -1;
> +		break;
>  	}
>
>  	free_cpu_drc_info(&dr_info);
[...]

> diff --git a/src/serv_config.c b/src/serv_config.c
> index 46e02f3..900f0f3 100644
> --- a/src/serv_config.c
> +++ b/src/serv_config.c
> @@ -463,7 +463,7 @@ int
>  update_nvram(char *var, char *val, char *partition) {
>  	char buf[256];
>  	pid_t child;
> -	int status, rc;
> +	int status;
>  	char *nvram_args[] = { "nvram", "--update-config",
>  			buf, "-p", partition, NULL };
>
> @@ -482,12 +482,12 @@ update_nvram(char *var, char *val, char *partition) {
>  	}
>  	else if (child == 0) {
>  		/* child process */
> -		rc = execv(NVRAM_PROGRAM, nvram_args);
> -
> -		/* shouldn't get here */
> -		err_msg(ERR_MSG, "Could not exec %s to update NVRAM\n",
> +		if (execv(NVRAM_PROGRAM, nvram_args)) {
> +			/* shouldn't get here */
> +			err_msg(ERR_MSG, "Could not exec %s to update NVRAM\n",
>  				NVRAM_PROGRAM);
> -		exit(1);
> +			exit(1);
> +		}
>  	}
>  	else {
>  		/* parent process */
> @@ -609,12 +609,11 @@ byte_to_string(uint8_t num, char *buf, size_t size) {
>   */
>  int
>  parse_call_home_buffer(char *var, char *buf, size_t size) {
> -	int buf_size;
>  	char *loc;
>
>  	if (!call_home_buffer) return RC_OTHER;	/* should never happen */
>
> -	buf_size = be16toh(*(uint16_t *)call_home_buffer);
> +	/* The first 16 bits is the call home bufer size, skip past this. */

Spelling of 'buffer' is misspelled.

>  	loc = call_home_buffer + sizeof(uint16_t);
>
>  	while (loc[0] != '\0') {
>

-- 
Cheers,
Kamalesh.