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

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

On Mon, 01 Aug 2011 18:26:23 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> 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.

Most of the libc functions should return -1 on error and set errno right
? and i guess most of the 9p function caller virtio_p9_error_reply with
errno as argument 

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

For the linux client we don't need the file to be open. There are few
issues here. 

1) You are only updating fid->fd, But fid->path is still the old path
2) You are following symlink on host. where as it should actually be
followed on the guest. (for ex: if we do ln -s /etc/a k on guest, we
actually want to open /etc/a on guest not the one on host.)

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

Won't that return ENOENT ?

lstat("k", 0x7fffd5d350c0)              = -1 ENOENT (No such file or directory)

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

-aneesh