Re: [PATCH 2/4] btrfs-progs: Integrate error message output into find_mount_root().

[Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>]

Hi Qu,

(2014/07/10 12:05), Qu Wenruo wrote:
> Before this patch, find_mount_root() and the caller both output error
> message, which sometimes make the output duplicated and hard to judge
> what the problem is.
> 
> This pathh will integrate all the error messages output into
> find_mount_root() to give more meaning error prompt and remove the
> unneeded caller error messages.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>   cmds-receive.c   |  2 --
>   cmds-send.c      |  8 +-------
>   cmds-subvolume.c |  5 +----
>   utils.c          | 15 ++++++++++++---
>   4 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/cmds-receive.c b/cmds-receive.c
> index 48380a5..084d97d 100644
> --- a/cmds-receive.c
> +++ b/cmds-receive.c
> @@ -981,8 +981,6 @@ static int do_receive(struct btrfs_receive *r, const char *tomnt, int r_fd,
>   	ret = find_mount_root(dest_dir_full_path, &r->root_path);
>   	if (ret < 0) {
>   		ret = -EINVAL;
> -		fprintf(stderr, "ERROR: failed to determine mount point "
> -			"for %s\n", dest_dir_full_path);
>   		goto out;
>   	}
>   	r->mnt_fd = open(r->root_path, O_RDONLY | O_NOATIME);
> diff --git a/cmds-send.c b/cmds-send.c
> index 9a73b32..091f32b 100644
> --- a/cmds-send.c
> +++ b/cmds-send.c
> @@ -357,8 +357,6 @@ static int init_root_path(struct btrfs_send *s, const char *subvol)
>   	ret = find_mount_root(subvol, &s->root_path);
>   	if (ret < 0) {
>   		ret = -EINVAL;
> -		fprintf(stderr, "ERROR: failed to determine mount point "
> -				"for %s\n", subvol);
>   		goto out;
>   	}
>   
> @@ -622,12 +620,8 @@ int cmd_send(int argc, char **argv)
>   		}
>   
>   		ret = find_mount_root(subvol, &mount_root);
> -		if (ret < 0) {
> -			fprintf(stderr, "ERROR: find_mount_root failed on %s: "
> -					"%s\n", subvol,
> -				strerror(-ret));
> +		if (ret < 0)
>   			goto out;
> -		}
>   		if (strcmp(send.root_path, mount_root) != 0) {
>   			ret = -EINVAL;
>   			fprintf(stderr, "ERROR: all subvols must be from the "
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index 639fb10..b252eab 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -981,11 +981,8 @@ static int cmd_subvol_show(int argc, char **argv)
>   	}
>   
>   	ret = find_mount_root(fullpath, &mnt);
> -	if (ret < 0) {
> -		fprintf(stderr, "ERROR: find_mount_root failed on %s: "
> -				"%s\n", fullpath, strerror(-ret));
> +	if (ret < 0)
>   		goto out;
> -	}
>   	ret = 1;
>   	svpath = get_subvol_name(mnt, fullpath);
>   
> diff --git a/utils.c b/utils.c
> index 507ec6c..07173ee 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -2417,13 +2417,19 @@ int find_mount_root(const char *path, char **mount_root)
>   	char *longest_match = NULL;
>   
>   	fd = open(path, O_RDONLY | O_NOATIME);
> -	if (fd < 0)
> +	if (fd < 0) {
> +		fprintf(stderr, "ERROR: Failed to open %s: %s\n",
> +			path, strerror(errno));

It drops part of original messages. It doesn't show this error
is from find_mount_root(). I consider the original meaning keep as is.
How do you think?

Thanks,
Satoru

>   		return -errno;
> +	}
>   	close(fd);
>   
>   	mnttab = setmntent("/proc/self/mounts", "r");
> -	if (!mnttab)
> +	if (!mnttab) {
> +		fprintf(stderr, "ERROR: Failed to setmntent: %s\n",
> +			strerror(errno));
>   		return -errno;
> +	}
>   
>   	while ((ent = getmntent(mnttab))) {
>   		len = strlen(ent->mnt_dir);
> @@ -2457,8 +2463,11 @@ int find_mount_root(const char *path, char **mount_root)
>   
>   	ret = 0;
>   	*mount_root = realpath(longest_match, NULL);
> -	if (!*mount_root)
> +	if (!*mount_root) {
> +		fprintf(stderr, "Failed to resolve path %s: %s\n",
> +			longest_match, strerror(errno));
>   		ret = -errno;
> +	}
>   
>   	free(longest_match);
>   	return ret;
>