Re: [PATCH 1/2] kvm tools: Add support for 9p2000.u

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

On Mon,  1 Aug 2011 17:08:22 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> This patch adds support for the UNIX extensions to 9p2000.
> 
> Supporting thses extensions allow us to transperantly mount UNIX directories
> without missing features such as symlinks.
> 
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  tools/kvm/include/kvm/virtio-9p.h |    4 +-
>  tools/kvm/virtio/9p-pdu.c         |    6 ++-
>  tools/kvm/virtio/9p.c             |   75 +++++++++++++++++++++++++++++++------
>  3 files changed, 69 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/kvm/include/kvm/virtio-9p.h b/tools/kvm/include/kvm/virtio-9p.h
> index 8584f49..0e55e5c 100644
> --- a/tools/kvm/include/kvm/virtio-9p.h
> +++ b/tools/kvm/include/kvm/virtio-9p.h
> @@ -11,8 +11,8 @@
>  #define VIRTQUEUE_NUM		128
>  #define	VIRTIO_P9_DEFAULT_TAG	"kvm_9p"
>  #define VIRTIO_P9_HDR_LEN	(sizeof(u32)+sizeof(u8)+sizeof(u16))
> -#define VIRTIO_P9_MAX_FID	128
> -#define VIRTIO_P9_VERSION	"9P2000"
> +#define VIRTIO_P9_MAX_FID	256
> +#define VIRTIO_P9_VERSION	"9P2000.u"
>  #define MAX_TAG_LEN		32
> 
>  struct p9_msg {
> diff --git a/tools/kvm/virtio/9p-pdu.c b/tools/kvm/virtio/9p-pdu.c
> index 0c454db..8ed249f 100644
> --- a/tools/kvm/virtio/9p-pdu.c
> +++ b/tools/kvm/virtio/9p-pdu.c
> @@ -200,13 +200,15 @@ static int virtio_p9_pdu_encode(struct p9_pdu *pdu, const char *fmt, va_list ap)
>  		case 'S':
>  		{
>  			struct p9_wstat *stbuf = va_arg(ap, struct p9_wstat *);
> -			retval = virtio_p9_pdu_writef(pdu, "wwdQdddqssss",
> +			retval = virtio_p9_pdu_writef(pdu, "wwdQdddqsssssddd",
>  						stbuf->size, stbuf->type,
>  						stbuf->dev, &stbuf->qid,
>  						stbuf->mode, stbuf->atime,
>  						stbuf->mtime, stbuf->length,
>  						stbuf->name, stbuf->uid,
> -						stbuf->gid, stbuf->muid);
> +						stbuf->gid, stbuf->muid,
> +						stbuf->extension, stbuf->n_uid,
> +						stbuf->n_gid, stbuf->n_muid);
>  		}
>  		break;
>  		default:
> diff --git a/tools/kvm/virtio/9p.c b/tools/kvm/virtio/9p.c
> index 3b5555c..a6eafe0 100644
> --- a/tools/kvm/virtio/9p.c
> +++ b/tools/kvm/virtio/9p.c
> @@ -162,7 +162,7 @@ static void virtio_p9_error_reply(struct p9_dev *p9dev,
> 
>  	err_str = strerror(err);
>  	pdu->write_offset = VIRTIO_P9_HDR_LEN;
> -	virtio_p9_pdu_writef(pdu, "s", err_str);
> +	virtio_p9_pdu_writef(pdu, "sd", err_str, -err);

Should that be -err ? I guess it should the errno value ?

>  	*outlen = pdu->write_offset;
> 
>  	pdu->read_offset = sizeof(u32) + sizeof(u8);
> @@ -204,7 +204,6 @@ static void virtio_p9_open(struct p9_dev *p9dev,
>  	struct p9_qid qid;
>  	struct p9_fid *new_fid;
> 
> -
>  	virtio_p9_pdu_readf(pdu, "db", &fid, &mode);
>  	new_fid = &p9dev->fids[fid];
> 
> @@ -242,13 +241,14 @@ static void virtio_p9_create(struct p9_dev *p9dev,
>  	u8 mode;
>  	u32 perm;
>  	char *name;
> +	char *ext = NULL;
>  	u32 fid_val;
>  	struct stat st;
>  	struct p9_qid qid;
>  	struct p9_fid *fid;
>  	char full_path[PATH_MAX];
> 
> -	virtio_p9_pdu_readf(pdu, "dsdb", &fid_val, &name, &perm, &mode);
> +	virtio_p9_pdu_readf(pdu, "dsdbs", &fid_val, &name, &perm, &mode,
> &ext);

ext need to be freed. ? I guess we have other places where the memory
alloc did for readf(.., "s", ..) is not freed. 

>  	fid = &p9dev->fids[fid_val];
> 
>  	sprintf(full_path, "%s/%s", fid->abs_path, name);
> @@ -262,6 +262,30 @@ static void virtio_p9_create(struct p9_dev *p9dev,
>  		close_fid(p9dev, fid_val);
>  		fid->dir = dir;
>  		fid->is_dir = 1;
> +	} else if (perm & P9_DMSYMLINK) {
> +		int r;
> +
> +		r = symlink(ext, full_path);
> +		if (r < 0)
> +			goto err_out;
> +		fd = open(full_path, omode2uflags(mode));
> +		if (fd < 0)
> +			goto err_out;
> +		close_fid(p9dev, fid_val);
> +		fid->fd = fd;


that updates the fid ? Is that needed ? Also above we are just updating
fid->fd where as fid->path still points to the old name.  I guess
symlink and link don't result in open 

> +	} else if (perm & P9_DMLINK) {
> +		int r;
> +		int ext_fid = atoi(ext);
> +
> +		r = link(p9dev->fids[ext_fid].abs_path, full_path);
> +		if (r < 0)
> +			goto err_out;
> +
> +		fd = open(full_path, omode2uflags(mode));
> +		if (fd < 0)
> +			goto err_out;
> +		close_fid(p9dev, fid_val);
> +		fid->fd = fd;
>  	} else {
>  		fd = open(full_path, omode2uflags(mode) | O_CREAT, 0777);
>  		if (fd < 0)
> @@ -294,7 +318,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
>  	u32 newfid_val;
>  	struct p9_qid wqid;
>  	struct p9_fid *new_fid;
> -
> +	int ret;
> 
>  	virtio_p9_pdu_readf(pdu, "ddw", &fid_val, &newfid_val, &nwname);
>  	new_fid	= &p9dev->fids[newfid_val];
> @@ -315,8 +339,10 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
>  			/* Format the new path we're 'walk'ing into */
>  			sprintf(tmp, "%s/%.*s",
>  				fid->path, (int)strlen(str), str);
> -			if (lstat(rel_to_abs(p9dev, tmp, full_path), &st) < 0)
> +			if (lstat(rel_to_abs(p9dev, tmp, full_path), &st) < 0) {
> +				ret = ENOENT;
>  				goto err_out;
> +			}

Why ?. What error with lstat failed which we wanted to map to ENOENT ?

> 
>  			st2qid(&st, &wqid);
>  			new_fid->is_dir = S_ISDIR(st.st_mode);
> @@ -340,7 +366,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
>  	virtio_p9_set_reply_header(pdu, *outlen);
>  	return;
>  err_out:
> -	virtio_p9_error_reply(p9dev, pdu, errno, outlen);
> +	virtio_p9_error_reply(p9dev, pdu, ret, outlen);
>  	return;
>  }
> 
> @@ -381,6 +407,12 @@ err_out:
>  	return;
>  }
> 
> +static void virtio_p9_free_stat(struct p9_wstat *wstat)
> +{
> +	free(wstat->extension);
> +	free(wstat->name);
> +}
> +
>  static void virtio_p9_fill_stat(struct p9_dev *p9dev, const char *name,
>  				struct stat *st, struct p9_wstat *wstat)
>  {
> @@ -393,6 +425,17 @@ static void virtio_p9_fill_stat(struct p9_dev *p9dev, const char *name,
>  		wstat->length = 0;
>  		wstat->mode |= P9_DMDIR;
>  	}
> +	if(S_ISLNK(st->st_mode)) {
> +		char tmp[PATH_MAX] = {0}, full_path[PATH_MAX] = {0};
> +
> +		rel_to_abs(p9dev, name, full_path);
> +
> +		if (readlink(full_path, tmp, PATH_MAX) > 0)
> +			wstat->extension = strdup(tmp);
> +		wstat->mode |= P9_DMSYMLINK;
> +	} else {
> +		wstat->extension = NULL;
> +	}
> 
>  	wstat->atime = st->st_atime;
>  	wstat->mtime = st->st_mtime;
> @@ -401,14 +444,20 @@ static void virtio_p9_fill_stat(struct p9_dev *p9dev, const char *name,
>  	wstat->uid = NULL;
>  	wstat->gid = NULL;
>  	wstat->muid = NULL;
> +	wstat->n_uid = wstat->n_gid = wstat->n_muid = 0;
> 
> -	/* NOTE: size shouldn't include its own length */
> -	/* size[2] type[2] dev[4] qid[13] */
> -	/* mode[4] atime[4] mtime[4] length[8]*/
> -	/* name[s] uid[s] gid[s] muid[s] */
> -	wstat->size = 2+4+13+4+4+4+8+2+2+2+2;
> +	/*
> +	 * NOTE: size shouldn't include its own length
> +	 * size[2] type[2] dev[4] qid[13]
> +	 * mode[4] atime[4] mtime[4] length[8]
> +	 * name[s] uid[s] gid[s] muid[s]
> +	 * ext[s] uid[4] gid[4] muid[4]
> +	 */
> +	wstat->size = 2+4+13+4+4+4+8+2+2+2+2+2+4+4+4;
>  	if (wstat->name)
>  		wstat->size += strlen(wstat->name);
> +	if (wstat->extension)
> +		wstat->size += strlen(wstat->extension);
>  }
> 
>  static void virtio_p9_read(struct p9_dev *p9dev,
> @@ -440,6 +489,7 @@ static void virtio_p9_read(struct p9_dev *p9dev,
>  			read = pdu->write_offset;
>  			virtio_p9_pdu_writef(pdu, "S", &wstat);
>  			rcount += pdu->write_offset - read;
> +			virtio_p9_free_stat(&wstat);
> 
>  			cur = readdir(fid->dir);
>  		}
> @@ -486,6 +536,7 @@ static void virtio_p9_stat(struct p9_dev *p9dev,
> 
>  	virtio_p9_pdu_writef(pdu, "wS", 0, &wstat);
>  	*outlen = pdu->write_offset;
> +	virtio_p9_free_stat(&wstat);
>  	virtio_p9_set_reply_header(pdu, *outlen);
>  	return;
>  err_out:
> -- 
> 1.7.6
> 

-aneesh