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

[Sasha Levin <levinsasha928@gmail.com>]

On Mon, 2011-08-01 at 20:42 +0530, Aneesh Kumar K.V wrote:
> 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 ?

Yup, but usually we get -errno back from functions and pass it to
virtio_p9_error_reply() while 9p expects it to be errno.

> 
> >  	*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. 

Oh heh, I took care of the part where we create wstat and write it, not
for the part where we read it :)

> 
> >  	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 

I'm really not sure if it's needed, the RFC isn't specific as to whether
we open 'special' files after creation or not, so I wanted to go with
opening them just in case (the kernel will clunk them anyway later, so
what the hell...).

Regarding the open in the symlink case, it's same deal really - I wasn't
sure whether create opens the file or not, and since theres no point in
opening a symlink I went ahead and opened the target of the symlink.

I didn't see a case where the kernel uses a linked/symlinked file
straight away, but as the RFC wasn't specific it was implemented anyway.

> 
> > +	} 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 ?

stat of non-existent files.
 
> 
> > 
> >  			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

-- 

Sasha.