On Mon, 01 Aug 2011 18:26:23 +0300, Sasha Levin <levinsasha...@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 <levinsasha...@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 <levinsasha...@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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
