Re: [PATCH 7/7] kvmtool: 9p: refactor rel_to_abs()

[Will Deacon <will.deacon@arm.com>]

On Tue, Oct 18, 2016 at 06:03:05PM +0200, G. Campana wrote:
> ---
>  virtio/9p.c | 50 +++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/virtio/9p.c b/virtio/9p.c
> index 5b2d261..3259b79 100644
> --- a/virtio/9p.c
> +++ b/virtio/9p.c
> @@ -91,18 +91,6 @@ static struct p9_fid *get_fid(struct p9_dev *p9dev, int fid)
>  	return new;
>  }
>  
> -static int rel_to_abs(struct p9_dev *p9dev, const char *path, char *abs_path,
> -		      size_t size)
> -{
> -	int ret;
> -
> -	ret = snprintf(abs_path, size, "%s/%s", p9dev->root_dir, path);
> -	if (ret >= (int)size)
> -		return -1;
> -
> -	return 0;
> -}

Can this be merged with patch 5, where you introduced rel_to_abs?

>  static void stat2qid(struct stat *st, struct p9_qid *qid)
>  {
>  	*qid = (struct p9_qid) {
> @@ -266,6 +254,28 @@ static int get_full_path(char *full_path, size_t size, struct p9_fid *fid,
>  	return 0;
>  }
>  
> +static int stat_rel(struct p9_dev *p9dev, const char *path, struct stat *st)
> +{
> +	int ret;
> +	char full_path[PATH_MAX];
> +
> +	ret = snprintf(full_path, sizeof(full_path), "%s/%s", p9dev->root_dir, path);
> +	if (ret >= (int)sizeof(full_path)) {
> +		errno = ENAMETOOLONG;
> +		return -1;
> +	}
> +
> +	if (path_is_illegal(full_path)) {
> +		errno = EACCES;
> +		return -1;
> +	}

Up to this point, you've just reimplemented most of get_full_path. Is it
worth giving these two functions a comment "concatenate these two path
components and check if the result is legal" backend?

> +	if (lstat(full_path, st) != 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
>  static void virtio_p9_open(struct p9_dev *p9dev,
>  			   struct p9_pdu *pdu, u32 *outlen)
>  {
> @@ -440,7 +450,6 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
>  		for (i = 0; i < nwname; i++) {
>  			struct stat st;
>  			char tmp[PATH_MAX] = {0};
> -			char full_path[PATH_MAX];
>  			char *str;
>  			int ret;
>  
> @@ -455,12 +464,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
>  
>  			free(str);
>  
> -			if (rel_to_abs(p9dev, tmp, full_path, sizeof(full_path)) != 0) {
> -				errno = ENAMETOOLONG;
> -				goto err_out;
> -			}
> -
> -			if (lstat(full_path, &st) < 0)
> +			if (stat_rel(p9dev, tmp, &st) != 0)
>  				goto err_out;
>  
>  			stat2qid(&st, &wqid);
> @@ -614,7 +618,6 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
>  	struct stat st;
>  	struct p9_fid *fid;
>  	struct dirent *dent;
> -	char full_path[PATH_MAX];
>  	u64 offset, old_offset;
>  
>  	rcount = 0;
> @@ -645,11 +648,8 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
>  			break;
>  		}
>  		old_offset = dent->d_off;
> -		if (rel_to_abs(p9dev, dent->d_name, full_path, sizeof(full_path)) != 0) {
> -			errno = ENAMETOOLONG;
> -			goto err_out;
> -		}
> -		lstat(full_path, &st);
> +		if (stat_rel(p9dev, dent->d_name, &st) != 0)
> +			memset(&st, -1, sizeof(st));

Why the memset, and not goto err_out?

Will