[Cluster-devel] [PATCH v2] libgfs2: Remove 'die' calls from __get_sysfs

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

Ok, that looks good now,

Steve.

On Thu, 2009-03-19 at 16:16 +0000, Andrew Price wrote:
> This patch removes the calls to die from __get_sysfs and adds error
> handling to it and its callers. To enable this, the prototype of
> get_sysfs_uint had to be changed so that error status can be returned.
> Callers to get_sysfs and get_sysfs_uint are also updated to handle error
> cases where appropriate.
> 
> Signed-off-by: Andrew Price <andy@andrewprice.me.uk>
> ---
>  Thanks for the comments on the previous version, I'm glad one of us is wide
>  awake today :-)
> 
>  gfs2/libgfs2/libgfs2.h |    2 +-
>  gfs2/libgfs2/misc.c    |   43 +++++++++++++++++++++++++++++++++----------
>  gfs2/tool/df.c         |   44 +++++++++++++++++++++++++++++++-------------
>  gfs2/tool/tune.c       |   10 ++++++++--
>  4 files changed, 73 insertions(+), 26 deletions(-)
> 
> diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
> index 2e79ad4..d8faf45 100644
> --- a/gfs2/libgfs2/libgfs2.h
> +++ b/gfs2/libgfs2/libgfs2.h
> @@ -634,7 +634,7 @@ extern char *find_debugfs_mount(void);
>  extern char *mp2fsname(char *mp);
>  extern char *mp2fsname2(char *devname);
>  extern char *get_sysfs(char *fsname, char *filename);
> -extern unsigned int get_sysfs_uint(char *fsname, char *filename);
> +extern int get_sysfs_uint(char *fsname, char *filename, unsigned int *val);
>  extern int set_sysfs(char *fsname, char *filename, char *val);
>  extern int is_fsname(char *name);
>  
> diff --git a/gfs2/libgfs2/misc.c b/gfs2/libgfs2/misc.c
> index 320ce84..60b2d4f 100644
> --- a/gfs2/libgfs2/misc.c
> +++ b/gfs2/libgfs2/misc.c
> @@ -216,10 +216,12 @@ static char *__get_sysfs(char *fsname, char *filename)
>  
>  	fd = open(path, O_RDONLY);
>  	if (fd < 0)
> -		die("can't open %s: %s\n", path, strerror(errno));
> +		return NULL;
>  	rv = read(fd, sysfs_buf, PAGE_SIZE);
> -	if (rv < 0)
> -		die("can't read from %s: %s\n", path, strerror(errno));
> +	if (rv < 0) {
> +		close(fd);
> +		return NULL;
> +	}
>  
>  	close(fd);
>  	return sysfs_buf;
> @@ -227,17 +229,30 @@ static char *__get_sysfs(char *fsname, char *filename)
>  
>  char *get_sysfs(char *fsname, char *filename)
>  {
> -	char *p = strchr(__get_sysfs(fsname, filename), '\n');
> +	char *s;
> +	char *p;
> +
> +	s = __get_sysfs(fsname, filename);
> +	if (!s)
> +		return NULL;
> +	p = strchr(s, '\n');
>  	if (p)
>  		*p = '\0';
>  	return sysfs_buf;
>  }
>  
> -unsigned int get_sysfs_uint(char *fsname, char *filename)
> +int get_sysfs_uint(char *fsname, char *filename, unsigned int *val)
>  {
> -	unsigned int x;
> -	sscanf(__get_sysfs(fsname, filename), "%u", &x);
> -	return x;
> +	char *s = __get_sysfs(fsname, filename);
> +	int ret;
> +	if (!s)
> +		return -1;
> +	ret = sscanf(s, "%u", val);
> +	if (1 != ret) {
> +		errno = ENOMSG;
> +		return -1;
> +	}
> +	return 0;
>  }
>  
>  int set_sysfs(char *fsname, char *filename, char *val)
> @@ -313,6 +328,7 @@ mp2fsname2(char *mp)
>  	char buffer[PATH_MAX], device_name[PATH_MAX];
>  	int fsdump, fspass, ret, found = 0;
>  	char fspath[PATH_MAX], fsoptions[PATH_MAX], fstype[80];
> +	char *id;
>  
>  	/* Take care of trailing '/' */
>  	if (mp[strlen(mp) - 1] == '/')
> @@ -361,7 +377,10 @@ mp2fsname2(char *mp)
>  		if (de->d_name[0] == '.')
>  			continue;
>  
> -		if (strcmp(get_sysfs(de->d_name, "id"), device_id) == 0) {
> +		id = get_sysfs(de->d_name, "id");
> +		if (!id)
> +			continue;
> +		if (strcmp(id, device_id) == 0) {
>  			fsname = strdup(de->d_name);
>  			break;
>  		}
> @@ -399,6 +418,7 @@ char *mp2fsname(char *mp)
>  	struct stat statbuf;
>  	DIR *d;
>  	struct dirent *de;
> +	char *id;
>  
>  	if (stat(mp, &statbuf))
>  		return NULL;
> @@ -415,7 +435,10 @@ char *mp2fsname(char *mp)
>  		if (de->d_name[0] == '.')
>  			continue;
>  
> -		if (strcmp(get_sysfs(de->d_name, "id"), device_id) == 0) {
> +		id = get_sysfs(de->d_name, "id");
> +		if (!id)
> +			continue;
> +		if (strcmp(id, device_id) == 0) {
>  			fsname = strdup(de->d_name);
>  			break;
>  		}
> diff --git a/gfs2/tool/df.c b/gfs2/tool/df.c
> index 7d2875d..c817813 100644
> --- a/gfs2/tool/df.c
> +++ b/gfs2/tool/df.c
> @@ -147,22 +147,40 @@ do_df_one(char *path)
>  	printf("  Block size = %u\n", sbd.sd_sb.sb_bsize);
>  	printf("  Journals = %u\n", journals);
>  	printf("  Resource Groups = %"PRIu64"\n", rgrps);
> -	printf("  Mounted lock proto = \"%s\"\n",
> -	       ((value = get_sysfs(fs, "args/lockproto"))[0])
> -	       ? value : sbd.sd_sb.sb_lockproto);
> -	printf("  Mounted lock table = \"%s\"\n",
> -	       ((value = get_sysfs(fs, "args/locktable"))[0])
> -	       ? value : sbd.sd_sb.sb_locktable);
> +	value = get_sysfs(fs, "args/lockproto");
> +	if (value)
> +		printf("  Mounted lock proto = \"%s\"\n",
> +			value[0] ? value : sbd.sd_sb.sb_lockproto);
> +	else
> +		printf("  Mounted lock proto = (Not found: %s)\n",
> +				strerror(errno));
> +
> +	value = get_sysfs(fs, "args/locktable");
> +	if (value)
> +		printf("  Mounted lock table = \"%s\"\n",
> +			value[0] ? value : sbd.sd_sb.sb_locktable);
> +	else
> +		printf("  Mounted lock table = (Not found: %s)\n",
> +				strerror(errno));
> +
>  	printf("  Mounted host data = \"%s\"\n",
>  	       get_sysfs(fs, "args/hostdata"));
>  	printf("  Journal number = %s\n", get_sysfs(fs, "lockstruct/jid"));
> -	flags = get_sysfs_uint(fs, "lockstruct/flags");
> -	printf("  Lock module flags = %x", flags);
> -	printf("\n");
> -	printf("  Local flocks = %s\n",
> -	       (get_sysfs_uint(fs, "args/localflocks")) ? "TRUE" : "FALSE");
> -	printf("  Local caching = %s\n",
> -		(get_sysfs_uint(fs, "args/localcaching")) ? "TRUE" : "FALSE");
> +
> +	if (get_sysfs_uint(fs, "lockstruct/flags", &flags))
> +		printf("  Lock module flags = (Not found: %s)\n", strerror(errno));
> +	else
> +		printf("  Lock module flags = %x\n", flags);
> +
> +	if (get_sysfs_uint(fs, "args/localflocks", &flags))
> +		printf("  Lock flocks = (Not found: %s)\n", strerror(errno));
> +	else
> +		printf("  Local flocks = %s\n", flags ? "TRUE" : "FALSE");
> +
> +	if (get_sysfs_uint(fs, "args/localcaching", &flags))
> +		printf("  Lock caching = (Not found: %s)\n", strerror(errno));
> +	else
> +		printf("  Local caching = %s\n", flags ? "TRUE" : "FALSE");
>  
>  	/* Read the master statfs file */
>  	if (mount_gfs2_meta(&sbd)) {
> diff --git a/gfs2/tool/tune.c b/gfs2/tool/tune.c
> index b4b24d8..29c529b 100644
> --- a/gfs2/tool/tune.c
> +++ b/gfs2/tool/tune.c
> @@ -39,6 +39,7 @@ get_tune(int argc, char **argv)
>  	double ratio;
>  	unsigned int num, den;
>  	struct gfs2_sbd sbd;
> +	char *value;
>  
>  	if (optind == argc)
>  		die("Usage: gfs2_tool gettune <mountpoint>\n");
> @@ -65,8 +66,13 @@ get_tune(int argc, char **argv)
>  			continue;
>  		snprintf(path, PATH_MAX - 1, "tune/%s", de->d_name);
>  		if (strcmp(de->d_name, "quota_scale") == 0) {
> -			sscanf(get_sysfs(fs, "tune/quota_scale"), "%u %u",
> -			       &num, &den);
> +			value = get_sysfs(fs, "tune/quota_scale");
> +			if (!value) {
> +				printf("quota_scale = (Not found: %s)\n",
> +						strerror(errno));
> +				continue;
> +			}
> +			sscanf(value, "%u %u", &num, &den);
>  			ratio = (double)num / den;
>  			printf("quota_scale = %.4f   (%u, %u)\n", ratio, num,
>  			       den);