Patchwork virtio-9p: Make rpath function thread safe

login
register
mail settings
Submitter Mohan Kumar M
Date April 15, 2011, 9:05 a.m.
Message ID <1302858357-11309-1-git-send-email-mohan@in.ibm.com>
Download mbox | patch
Permalink /patch/91346/
State New
Headers show

Comments

Mohan Kumar M - April 15, 2011, 9:05 a.m.
Use dynamically allocated memory to return concatenated path
(fs_root and file path) instead of a static buffer. Caller has to free
the memory.

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
This patch depends on my chroot patchset.
http://patchwork.ozlabs.org/patch/89033/

 hw/9pfs/virtio-9p-local.c |  551 ++++++++++++++++++++++++++-------------------
 hw/file-op-9p.h           |   10 +-
 2 files changed, 322 insertions(+), 239 deletions(-)
Stefan Hajnoczi - April 15, 2011, 9:42 a.m.
On Fri, Apr 15, 2011 at 10:05 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> Use dynamically allocated memory to return concatenated path
> (fs_root and file path) instead of a static buffer. Caller has to free
> the memory.
>
> Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
> ---
> This patch depends on my chroot patchset.
> http://patchwork.ozlabs.org/patch/89033/
>
>  hw/9pfs/virtio-9p-local.c |  551 ++++++++++++++++++++++++++-------------------
>  hw/file-op-9p.h           |   10 +-
>  2 files changed, 322 insertions(+), 239 deletions(-)

Would it be possible to build a const char *fs_ctx->path once in the
PDU handler for each operation that takes a path instead of pushing so
many rpath() calls down into virtio-9p-local.c?  That way you don't
need to worry about constructing and freeing paths over and over.

I think it's time to kill errno once and for all in virtio-9p.  The
errno global variable usage is getting out of hand; pretty much every
patch is adding boilerplate code to juggle around errno and avoid
trampling it.  This new qemu_free() call means we need to juggle in
new places.

Please stop adding more errno juggling and just use -errno return
values.  That change needs to happen before this patch.

> @@ -137,29 +164,10 @@ static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
>         }
>         if (getxattr(rpath(fs_ctx, path), "user.virtfs.rdev", &tmp_dev,
>                         sizeof(dev_t)) > 0) {

Leak.

>         tsize = readlink(rpath(fs_ctx, path), buf, bufsz);

Leak.

> -static void local_rewinddir(FsContext *ctx, DIR *dir)
> +static void local_rewinddir(FsContext *fs_ctx, DIR *dir)
>  {
>     return rewinddir(dir);
>  }

Please do this in a separate patch.

>         err = local_set_xattr(rpath(fs_ctx, newpath), credp);

Leak.

Stefan
jvrao - April 15, 2011, 3:02 p.m.
On 04/15/2011 02:42 AM, Stefan Hajnoczi wrote:
> On Fri, Apr 15, 2011 at 10:05 AM, M. Mohan Kumar<mohan@in.ibm.com>  wrote:
>> Use dynamically allocated memory to return concatenated path
>> (fs_root and file path) instead of a static buffer. Caller has to free
>> the memory.
>>
>> Signed-off-by: M. Mohan Kumar<mohan@in.ibm.com>
>> ---
>> This patch depends on my chroot patchset.
>> http://patchwork.ozlabs.org/patch/89033/
>>
>>   hw/9pfs/virtio-9p-local.c |  551 ++++++++++++++++++++++++++-------------------
>>   hw/file-op-9p.h           |   10 +-
>>   2 files changed, 322 insertions(+), 239 deletions(-)
> Would it be possible to build a const char *fs_ctx->path once in the
> PDU handler for each operation that takes a path instead of pushing so
> many rpath() calls down into virtio-9p-local.c?  That way you don't
> need to worry about constructing and freeing paths over and over.
This is very good suggestion. I think we should explore this.

> I think it's time to kill errno once and for all in virtio-9p.  The
> errno global variable usage is getting out of hand; pretty much every
> patch is adding boilerplate code to juggle around errno and avoid
> trampling it.  This new qemu_free() call means we need to juggle in
> new places.
>
> Please stop adding more errno juggling and just use -errno return
> values.  That change needs to happen before this patch.
This is ideal and we should do it where ever possible.

But I am not sure if this is really achievable.
In the case of passthrough we need to pass this information
from chroot() process to qemu. In other security models
we don't have 1-1 correspondence between 9p operations and system calls.
Will catch you on IRC for further discussion.

Thanks,
JV

>> @@ -137,29 +164,10 @@ static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
>>          }
>>          if (getxattr(rpath(fs_ctx, path), "user.virtfs.rdev",&tmp_dev,
>>                          sizeof(dev_t))>  0) {
> Leak.
>
>>          tsize = readlink(rpath(fs_ctx, path), buf, bufsz);
> Leak.
>
>> -static void local_rewinddir(FsContext *ctx, DIR *dir)
>> +static void local_rewinddir(FsContext *fs_ctx, DIR *dir)
>>   {
>>      return rewinddir(dir);
>>   }
> Please do this in a separate patch.
>
>>          err = local_set_xattr(rpath(fs_ctx, newpath), credp);
> Leak.
>
> Stefan
>

Patch

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index fa5c88f..abe3987 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -109,15 +109,42 @@  static int get_dirfd(FsContext *fs_ctx, const char *path)
     return fd;
 }
 
+static int passthrough_lstat(FsContext *fs_ctx, const char *path,
+                struct stat *stbuf)
+{
+    int pfd, retval, serrno = 0;
+    char *tmp_path;
+
+    pfd = get_dirfd(fs_ctx, path);
+    if (pfd < 0) {
+        return -1;
+    }
+    tmp_path = qemu_strdup(path);
+    retval = fstatat(pfd, basename(tmp_path), stbuf, AT_SYMLINK_NOFOLLOW);
+    serrno = retval < 0 ? errno : 0;
+    close(pfd);
+    qemu_free(tmp_path);
+    errno = serrno ? serrno : errno;
+    return retval;
+}
+
 static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
 {
-    int err;
+    int err, serrno = 0;
+    char *full_path;
 
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        return passthrough_lstat(fs_ctx, path, stbuf);
+    }
+    full_path = rpath(fs_ctx, path);
+    err = lstat(full_path, stbuf);
+    if (err < 0) {
+        serrno = errno;
+        qemu_free(full_path);
+        errno = serrno;
+        return err;
+    }
     if (fs_ctx->fs_sm == SM_MAPPED) {
-        err =  lstat(rpath(fs_ctx, path), stbuf);
-        if (err) {
-            return err;
-        }
         /* Actual credentials are part of extended attrs */
         uid_t tmp_uid;
         gid_t tmp_gid;
@@ -137,29 +164,10 @@  static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
         }
         if (getxattr(rpath(fs_ctx, path), "user.virtfs.rdev", &tmp_dev,
                         sizeof(dev_t)) > 0) {
-                stbuf->st_rdev = tmp_dev;
-        }
-    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
-        int pfd, serrno = 0;
-        char *tmp_path;
-
-        pfd = get_dirfd(fs_ctx, path);
-        if (pfd < 0) {
-            return -1;
-        }
-        tmp_path = qemu_strdup(path);
-        err = fstatat(pfd, basename(tmp_path), stbuf, AT_SYMLINK_NOFOLLOW);
-        if (err < 0) {
-            serrno = errno;
-        }
-        close(pfd);
-        qemu_free(tmp_path);
-        if (err < 0) {
-            errno = serrno;
+            stbuf->st_rdev = tmp_dev;
         }
-    } else {
-        err =  lstat(rpath(fs_ctx, path), stbuf);
     }
+    qemu_free(full_path);
     return err;
 }
 
@@ -197,54 +205,62 @@  static int local_set_xattr(const char *path, FsCred *credp)
     return 0;
 }
 
-static int local_post_create_none(FsContext *fs_ctx, const char *path,
-        FsCred *credp)
+static void local_post_create_none(const char *path, FsCred *credp)
 {
-    int retval;
-    if (chmod(rpath(fs_ctx, path), credp->fc_mode & 07777) < 0) {
+    chmod(path, credp->fc_mode & 07777);
+    lchown(path, credp->fc_uid, credp->fc_gid);
+}
+
+static ssize_t passthrough_readlink(FsContext *fs_ctx, const char *path,
+        char *buf, size_t bufsz)
+{
+    int pfd, serrno = 0, tsize;
+    char *tmp_path;
+    pfd = get_dirfd(fs_ctx, path);
+    if (pfd < 0) {
         return -1;
     }
-    retval = lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
-    return 0;
+
+    tmp_path = qemu_strdup(path);
+    tsize = readlinkat(pfd, basename(tmp_path), buf, bufsz);
+    serrno = tsize < 0 ? errno : 0;
+    close(pfd);
+    qemu_free(tmp_path);
+    errno = serrno ? serrno : errno;
+    return tsize;
 }
 
 static ssize_t local_readlink(FsContext *fs_ctx, const char *path,
         char *buf, size_t bufsz)
 {
-    ssize_t tsize = -1;
+    ssize_t tsize = -1, serrno = 0;
+    char *full_path;
+
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        return passthrough_readlink(fs_ctx, path, buf, bufsz);
+    }
+    full_path = rpath(fs_ctx, path);
     if (fs_ctx->fs_sm == SM_MAPPED) {
         int fd;
-        fd = open(rpath(fs_ctx, path), O_RDONLY);
-        if (fd == -1) {
-            return -1;
+        fd = open(full_path, O_RDONLY);
+        if (fd < 0) {
+            serrno = errno;
+            goto err_end;
         }
         do {
             tsize = read(fd, (void *)buf, bufsz);
         } while (tsize == -1 && errno == EINTR);
+        serrno = tsize < 0 ? errno : serrno;
         close(fd);
-        return tsize;
     } else if (fs_ctx->fs_sm == SM_NONE) {
         tsize = readlink(rpath(fs_ctx, path), buf, bufsz);
-    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
-        int pfd, serrno = 0;
-        char *tmp_path;
-        pfd = get_dirfd(fs_ctx, path);
-        if (pfd < 0) {
-            return -1;
-        }
-
-        tmp_path = qemu_strdup(path);
-        tsize = readlinkat(pfd, basename(tmp_path), buf, bufsz);
-        if (tsize < 0) {
-            serrno = errno;
-        }
-        close(pfd);
-        qemu_free(tmp_path);
-        if (tsize < 0) {
-            errno = serrno;
-        }
     }
+    qemu_free(full_path);
     return tsize;
+err_end:
+    qemu_free(full_path);
+    errno = serrno;
+    return -1;
 }
 
 static int local_close(FsContext *ctx, int fd)
@@ -259,15 +275,25 @@  static int local_closedir(FsContext *ctx, DIR *dir)
 
 static int local_open(FsContext *fs_ctx, const char *path, int flags)
 {
+    char *full_path;
+    int fd, serrno = 0;
     if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
         return passthrough_request(fs_ctx, NULL, path, flags, NULL, T_OPEN);
-    } else {
-        return open(rpath(fs_ctx, path), flags);
     }
+    full_path = rpath(fs_ctx, path);
+    fd = open(full_path, flags);
+    serrno = fd < 0 ? errno : 0;
+    qemu_free(full_path);
+    errno = serrno ? serrno : errno;
+    return fd;
 }
 
 static DIR *local_opendir(FsContext *fs_ctx, const char *path)
 {
+    char *full_path;
+    int serrno = 0;
+    DIR *retval;
+
     if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
         int fd;
         fd = passthrough_request(fs_ctx, NULL, path, O_DIRECTORY, NULL, T_OPEN);
@@ -275,32 +301,36 @@  static DIR *local_opendir(FsContext *fs_ctx, const char *path)
             return NULL;
         }
         return fdopendir(fd);
-    } else {
-        return opendir(rpath(fs_ctx, path));
     }
+    full_path = rpath(fs_ctx, path);
+    retval = opendir(full_path);
+    serrno = retval == NULL ? errno : 0;
+    qemu_free(full_path);
+    errno = serrno ? serrno : errno;
+    return retval;
 }
 
-static void local_rewinddir(FsContext *ctx, DIR *dir)
+static void local_rewinddir(FsContext *fs_ctx, DIR *dir)
 {
     return rewinddir(dir);
 }
 
-static off_t local_telldir(FsContext *ctx, DIR *dir)
+static off_t local_telldir(FsContext *fs_ctx, DIR *dir)
 {
     return telldir(dir);
 }
 
-static struct dirent *local_readdir(FsContext *ctx, DIR *dir)
+static struct dirent *local_readdir(FsContext *fs_ctx, DIR *dir)
 {
     return readdir(dir);
 }
 
-static void local_seekdir(FsContext *ctx, DIR *dir, off_t off)
+static void local_seekdir(FsContext *fs_ctx, DIR *dir, off_t off)
 {
     return seekdir(dir, off);
 }
 
-static ssize_t local_preadv(FsContext *ctx, int fd, const struct iovec *iov,
+static ssize_t local_preadv(FsContext *fs_ctx, int fd, const struct iovec *iov,
                             int iovcnt, off_t offset)
 {
 #ifdef CONFIG_PREADV
@@ -315,7 +345,7 @@  static ssize_t local_preadv(FsContext *ctx, int fd, const struct iovec *iov,
 #endif
 }
 
-static ssize_t local_pwritev(FsContext *ctx, int fd, const struct iovec *iov,
+static ssize_t local_pwritev(FsContext *fs_ctx, int fd, const struct iovec *iov,
                             int iovcnt, off_t offset)
 {
 #ifdef CONFIG_PREADV
@@ -332,87 +362,102 @@  static ssize_t local_pwritev(FsContext *ctx, int fd, const struct iovec *iov,
 
 static int local_chmod(FsContext *fs_ctx, const char *path, FsCred *credp)
 {
-    if (fs_ctx->fs_sm == SM_MAPPED) {
-        return local_set_xattr(rpath(fs_ctx, path), credp);
-    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+    char *full_path;
+    int serrno = 0, retval;
+
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
         return passthrough_request(fs_ctx, NULL, path, 0, credp, T_CHMOD);
-    } else if (fs_ctx->fs_sm == SM_NONE) {
-        return chmod(rpath(fs_ctx, path), credp->fc_mode);
     }
-    return -1;
+
+    full_path = rpath(fs_ctx, path);
+    if (fs_ctx->fs_sm == SM_MAPPED) {
+        retval = local_set_xattr(full_path, credp);
+    } else {
+        retval = chmod(full_path, credp->fc_mode);
+    }
+    serrno = retval < 0 ? errno : 0;
+    qemu_free(full_path);
+    errno = serrno ? serrno : errno;
+    return retval;
 }
 
 static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp)
 {
-    int err = -1;
-    int serrno = 0;
+    int err = -1, serrno = 0;
+    char *full_path;
 
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        return passthrough_request(fs_ctx, NULL, path, 0, credp, T_MKNOD);
+    }
+
+    full_path = rpath(fs_ctx, path);
     /* Determine the security model */
     if (fs_ctx->fs_sm == SM_MAPPED) {
-        err = mknod(rpath(fs_ctx, path), SM_LOCAL_MODE_BITS|S_IFREG, 0);
-        if (err == -1) {
-            return err;
-        }
-        local_set_xattr(rpath(fs_ctx, path), credp);
-        if (err == -1) {
+        err = mknod(full_path, SM_LOCAL_MODE_BITS|S_IFREG, 0);
+        if (err < 0) {
             serrno = errno;
             goto err_end;
         }
-    } else if (fs_ctx->fs_sm == SM_NONE) {
-        err = mknod(rpath(fs_ctx, path), credp->fc_mode, credp->fc_rdev);
-        if (err == -1) {
-            return err;
+        err = local_set_xattr(full_path, credp);
+        if (err < 0) {
+            serrno = errno;
+            goto err_rm;
         }
-        err = local_post_create_none(fs_ctx, path, credp);
-        if (err == -1) {
+    } else if (fs_ctx->fs_sm == SM_NONE) {
+        err = mknod(full_path, credp->fc_mode, credp->fc_rdev);
+        if (err < 0) {
             serrno = errno;
             goto err_end;
         }
-    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
-        err = passthrough_request(fs_ctx, NULL, path, 0, credp, T_MKNOD);
+        local_post_create_none(full_path, credp);
     }
+    qemu_free(full_path);
     return err;
-
+err_rm:
+    remove(full_path);
 err_end:
-    remove(rpath(fs_ctx, path));
+    qemu_free(full_path);
     errno = serrno;
     return err;
 }
 
 static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp)
 {
-    int err = -1;
-    int serrno = 0;
+    int err = -1, serrno = 0;
+    char *full_path;
+
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        return passthrough_request(fs_ctx, NULL, path, 0, credp, T_MKDIR);
+    }
 
+    full_path = rpath(fs_ctx, path);
     /* Determine the security model */
     if (fs_ctx->fs_sm == SM_MAPPED) {
-        err = mkdir(rpath(fs_ctx, path), SM_LOCAL_DIR_MODE_BITS);
-        if (err == -1) {
-            return err;
+        err = mkdir(full_path, SM_LOCAL_DIR_MODE_BITS);
+        if (err < 0) {
+            serrno = errno;
+            goto err_end;
         }
         credp->fc_mode = credp->fc_mode|S_IFDIR;
-        err = local_set_xattr(rpath(fs_ctx, path), credp);
-        if (err == -1) {
+        err = local_set_xattr(full_path, credp);
+        if (err < 0) {
             serrno = errno;
-            goto err_end;
+            goto err_rm;
         }
     } else if (fs_ctx->fs_sm == SM_NONE) {
-        err = mkdir(rpath(fs_ctx, path), credp->fc_mode);
-        if (err == -1) {
-            return err;
-        }
-        err = local_post_create_none(fs_ctx, path, credp);
-        if (err == -1) {
+        err = mkdir(full_path, credp->fc_mode);
+        if (err < 0) {
             serrno = errno;
             goto err_end;
         }
-    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
-        err = passthrough_request(fs_ctx, NULL, path, 0, credp, T_MKDIR);
+        local_post_create_none(full_path, credp);
     }
+    qemu_free(full_path);
     return err;
-
+err_rm:
+    remove(full_path);
 err_end:
-    remove(rpath(fs_ctx, path));
+    qemu_free(full_path);
     errno = serrno;
     return err;
 }
@@ -450,60 +495,68 @@  static int local_fstat(FsContext *fs_ctx, int fd, struct stat *stbuf)
 static int local_open2(FsContext *fs_ctx, const char *path, int flags,
         FsCred *credp)
 {
-    int fd = -1;
-    int err = -1;
-    int serrno = 0;
+    int fd = -1, err = -1, serrno = 0;
+    char *full_path;
+
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        return passthrough_request(fs_ctx, NULL, path, flags, credp, T_CREATE);
+    }
 
+    full_path = rpath(fs_ctx, path);
     /* Determine the security model */
     if (fs_ctx->fs_sm == SM_MAPPED) {
-        fd = open(rpath(fs_ctx, path), flags, SM_LOCAL_MODE_BITS);
-        if (fd == -1) {
-            return fd;
+        fd = open(full_path, flags, SM_LOCAL_MODE_BITS);
+        if (fd < 0) {
+            serrno = errno;
+            goto err_end;
         }
         credp->fc_mode = credp->fc_mode|S_IFREG;
-        /* Set cleint credentials in xattr */
-        err = local_set_xattr(rpath(fs_ctx, path), credp);
-        if (err == -1) {
+        /* Set client credentials in xattr */
+        err = local_set_xattr(full_path, credp);
+        if (err < 0) {
             serrno = errno;
-            goto err_end;
+            goto err_rm;
         }
     } else if (fs_ctx->fs_sm == SM_NONE) {
-        fd = open(rpath(fs_ctx, path), flags, credp->fc_mode);
-        if (fd == -1) {
-            return fd;
-        }
-        err = local_post_create_none(fs_ctx, path, credp);
-        if (err == -1) {
+        fd = open(full_path, flags, credp->fc_mode);
+        if (fd < 0) {
             serrno = errno;
             goto err_end;
         }
-    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
-        fd = passthrough_request(fs_ctx, NULL, path, flags, credp, T_CREATE);
+        local_post_create_none(full_path, credp);
     }
+    qemu_free(full_path);
     return fd;
-
+err_rm:
+    remove(full_path);
 err_end:
     close(fd);
-    remove(rpath(fs_ctx, path));
+    qemu_free(full_path);
     errno = serrno;
     return err;
 }
 
-
 static int local_symlink(FsContext *fs_ctx, const char *oldpath,
         const char *newpath, FsCred *credp)
 {
-    int err = -1;
-    int serrno = 0;
+    int err = -1, serrno = 0;
+    char *full_newpath;
+
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        err = passthrough_request(fs_ctx, oldpath, newpath, 0, credp,
+                        T_SYMLINK);
+    }
+
+    full_newpath = rpath(fs_ctx, newpath);
 
     /* Determine the security model */
     if (fs_ctx->fs_sm == SM_MAPPED) {
         int fd;
         ssize_t oldpath_size, write_size;
-        fd = open(rpath(fs_ctx, newpath), O_CREAT|O_EXCL|O_RDWR,
-                SM_LOCAL_MODE_BITS);
-        if (fd == -1) {
-            return fd;
+        fd = open(full_newpath, O_CREAT|O_EXCL|O_RDWR, SM_LOCAL_MODE_BITS);
+        if (fd < 0) {
+            errno = serrno;
+            goto err_end;
         }
         /* Write the oldpath (target) to the file. */
         oldpath_size = strlen(oldpath);
@@ -515,31 +568,30 @@  static int local_symlink(FsContext *fs_ctx, const char *oldpath,
             serrno = errno;
             close(fd);
             err = -1;
-            goto err_end;
+            goto err_rm;
         }
         close(fd);
-        /* Set cleint credentials in symlink's xattr */
+        /* Set client credentials in symlink's xattr */
         credp->fc_mode = credp->fc_mode|S_IFLNK;
         err = local_set_xattr(rpath(fs_ctx, newpath), credp);
-        if (err == -1) {
+        if (err < 0) {
             serrno = errno;
             goto err_end;
         }
     } else if (fs_ctx->fs_sm == SM_NONE) {
-        err = symlink(oldpath, rpath(fs_ctx, newpath));
-        if (err) {
-            return err;
+        err = symlink(oldpath, full_newpath);
+        if (err < 0) {
+            errno = serrno;
+            goto err_end;
         }
-        err = lchown(rpath(fs_ctx, newpath), credp->fc_uid, credp->fc_gid);
-        err = 0;
-    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
-        err = passthrough_request(fs_ctx, oldpath, newpath, 0, credp,
-                        T_SYMLINK);
+        (void)lchown(full_newpath, credp->fc_uid, credp->fc_gid);
     }
+    qemu_free(full_newpath);
     return err;
-
+err_rm:
+    remove(full_newpath);
 err_end:
-    remove(rpath(fs_ctx, newpath));
+    qemu_free(full_newpath);
     errno = serrno;
     return err;
 }
@@ -548,119 +600,149 @@  static int local_link(FsContext *fs_ctx, const char *oldpath,
                 const char *newpath)
 {
     int err, serrno = 0;
-    char *tmp;
+    char *full_oldpath, *full_newpath;
 
     if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
         return passthrough_request(fs_ctx, oldpath, newpath, 0, NULL, T_LINK);
     }
 
-    tmp = qemu_strdup(rpath(fs_ctx, oldpath));
-    err = link(tmp, rpath(fs_ctx, newpath));
-    if (err == -1) {
-        serrno = errno;
-    }
-    qemu_free(tmp);
-    if (err == -1) {
-        errno = serrno;
-    }
+    full_oldpath = rpath(fs_ctx, oldpath);
+    full_newpath = rpath(fs_ctx, newpath);
+    err = link(full_oldpath, full_newpath);
+    serrno = err < 0 ? errno : 0;
+    qemu_free(full_oldpath);
+    qemu_free(full_newpath);
+    errno = serrno ? serrno : errno;
     return err;
 }
 
-static int local_truncate(FsContext *ctx, const char *path, off_t size)
+static int passthrough_truncate(FsContext *fs_ctx, const char *path, off_t size)
 {
-    if (ctx->fs_sm == SM_PASSTHROUGH) {
-        int fd, retval, serrno;
-        fd = passthrough_request(ctx, NULL, path, O_RDWR, NULL, T_OPEN);
-        if (fd < 0) {
-            errno = -fd;
-            return -1;
-        }
-        retval = ftruncate(fd, size);
-        if (retval < 0) {
-            serrno = errno;
-        }
-        close(fd);
-        if (retval < 0) {
-            errno = serrno;
-        }
-        return retval;
-    } else {
-        return truncate(rpath(ctx, path), size);
+    int fd, retval, serrno;
+    fd = passthrough_request(fs_ctx, NULL, path, O_RDWR, NULL, T_OPEN);
+    if (fd < 0) {
+        errno = -fd;
+        return -1;
     }
+    retval = ftruncate(fd, size);
+    serrno = retval < 0 ? errno : 0;
+    close(fd);
+    errno = serrno ? serrno : errno;
+    return retval;
 }
 
-static int local_rename(FsContext *ctx, const char *oldpath,
+static int local_truncate(FsContext *fs_ctx, const char *path, off_t size)
+{
+    int serrno, retval;
+    char *full_path;
+
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        return passthrough_truncate(fs_ctx, path, size);
+    }
+
+    full_path = rpath(fs_ctx, path);
+    retval = truncate(full_path, size);
+    serrno = retval < 0 ? errno : 0;
+    qemu_free(full_path);
+    errno = serrno ? serrno : errno;
+    return retval;
+}
+
+static int local_rename(FsContext *fs_ctx, const char *oldpath,
                         const char *newpath)
 {
     int err, serrno;
-    char *tmp;
+    char *full_oldpath, *full_newpath;
 
-    if (ctx->fs_sm == SM_PASSTHROUGH) {
-        return passthrough_request(ctx, oldpath, newpath, 0, NULL, T_RENAME);
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        return passthrough_request(fs_ctx, oldpath, newpath, 0, NULL, T_RENAME);
     }
 
-    tmp = qemu_strdup(rpath(ctx, oldpath));
-    err = rename(tmp, rpath(ctx, newpath));
-    if (err == -1) {
-        serrno = errno;
-    }
-    qemu_free(tmp);
-    if (err == -1) {
-        errno = serrno;
-    }
+    full_oldpath = rpath(fs_ctx, oldpath);
+    full_newpath = rpath(fs_ctx, newpath);
+    err = rename(full_oldpath, full_newpath);
+    serrno = err < 0 ? errno : 0;
+    qemu_free(full_oldpath);
+    qemu_free(full_newpath);
+    errno = serrno ? serrno : errno;
     return err;
 }
 
 static int local_chown(FsContext *fs_ctx, const char *path, FsCred *credp)
 {
-    if (fs_ctx->fs_sm != SM_PASSTHROUGH &&
-            (credp->fc_uid == -1 && credp->fc_gid == -1)) {
-        return lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
-    } else if (fs_ctx->fs_sm == SM_MAPPED) {
-        return local_set_xattr(rpath(fs_ctx, path), credp);
-    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+    int retval, serrno;
+    char *full_path;
+
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
         return passthrough_request(fs_ctx, NULL, path, 0, credp, T_CHOWN);
-    } else if (fs_ctx->fs_sm == SM_NONE) {
-        return lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
     }
-    return -1;
+    full_path = rpath(fs_ctx, path);
+    if (credp->fc_uid == -1 && credp->fc_gid == -1) {
+        retval = lchown(full_path, credp->fc_uid, credp->fc_gid);
+    } else if (fs_ctx->fs_sm == SM_MAPPED) {
+        retval = local_set_xattr(full_path, credp);
+    } else {
+        retval = lchown(full_path, credp->fc_uid, credp->fc_gid);
+    }
+    serrno = retval < 0 ? errno : 0;
+    qemu_free(full_path);
+    errno = serrno ? serrno : 0;
+    return retval;
+}
+
+static int passthrough_utimensat(FsContext *fs_ctx, const char *path,
+                const struct timespec *buf)
+{
+    int fd, retval, serrno = 0;
+    fd = passthrough_request(fs_ctx, NULL, path, O_RDONLY | O_NONBLOCK |
+                                O_NOFOLLOW, NULL, T_OPEN);
+    if (fd < 0) {
+        errno = -fd;
+        return -1;
+    }
+    retval = futimens(fd, buf);
+    if (retval < 0) {
+        serrno = errno;
+    }
+    close(fd);
+    errno = serrno ? serrno : errno;
+    return retval;
 }
 
 static int local_utimensat(FsContext *fs_ctx, const char *path,
                 const struct timespec *buf)
 {
+    char *full_path;
+    int serrno, retval;
+
     if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
-        int fd, retval, serrno = 0;
-        fd = passthrough_request(fs_ctx, NULL, path,
-                        O_RDONLY | O_NONBLOCK | O_NOFOLLOW, NULL, T_OPEN);
-        if (fd < 0) {
-            errno = -fd;
-            return -1;
-        }
-        retval = futimens(fd, buf);
-        if (retval < 0) {
-            serrno = errno;
-        }
-        close(fd);
-        if (retval < 0) {
-            errno = serrno;
-        }
-        return retval;
-    } else {
-        return utimensat(AT_FDCWD, rpath(fs_ctx, path), buf,
-                        AT_SYMLINK_NOFOLLOW);
+        return passthrough_utimensat(fs_ctx, path, buf);
     }
+    full_path = rpath(fs_ctx, path);
+    retval = utimensat(AT_FDCWD, full_path, buf, AT_SYMLINK_NOFOLLOW);
+    serrno = retval < 0 ? errno : 0;
+    qemu_free(full_path);
+    errno = serrno ? serrno : errno;
+    return retval;
 }
 
-static int local_remove(FsContext *ctx, const char *path)
+static int local_remove(FsContext *fs_ctx, const char *path)
 {
-    if (ctx->fs_sm == SM_PASSTHROUGH) {
-        return passthrough_request(ctx, NULL, path, 0, NULL, T_REMOVE);
+    int serrno, retval;
+    char *full_path;
+
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        return passthrough_request(fs_ctx, NULL, path, 0, NULL, T_REMOVE);
     }
-    return remove(rpath(ctx, path));
+    full_path = rpath(fs_ctx, path);
+    retval = remove(full_path);
+    serrno = retval < 0 ? errno : 0;
+    qemu_free(full_path);
+    errno = serrno ? serrno : errno;
+    return retval;
 }
 
-static int local_fsync(FsContext *ctx, int fd, int datasync)
+static int local_fsync(FsContext *fs_ctx, int fd, int datasync)
 {
     if (datasync) {
         return qemu_fdatasync(fd);
@@ -669,36 +751,37 @@  static int local_fsync(FsContext *ctx, int fd, int datasync)
     }
 }
 
-static int local_statfs(FsContext *s, const char *path, struct statfs *stbuf)
+static int local_statfs(FsContext *fs_ctx, const char *path,
+                struct statfs *stbuf)
 {
-   return statfs(rpath(s, path), stbuf);
+   return statfs(fs_ctx->fs_root, stbuf);
 }
 
-static ssize_t local_lgetxattr(FsContext *ctx, const char *path,
+static ssize_t local_lgetxattr(FsContext *fs_ctx, const char *path,
                                const char *name, void *value, size_t size)
 {
-    return v9fs_get_xattr(ctx, path, name, value, size);
+    return v9fs_get_xattr(fs_ctx, path, name, value, size);
 }
 
-static ssize_t local_llistxattr(FsContext *ctx, const char *path,
+static ssize_t local_llistxattr(FsContext *fs_ctx, const char *path,
                                 void *value, size_t size)
 {
-    return v9fs_list_xattr(ctx, path, value, size);
+    return v9fs_list_xattr(fs_ctx, path, value, size);
 }
 
-static int local_lsetxattr(FsContext *ctx, const char *path, const char *name,
-                           void *value, size_t size, int flags)
+static int local_lsetxattr(FsContext *fs_ctx, const char *path,
+                const char *name, void *value, size_t size, int flags)
 {
-    return v9fs_set_xattr(ctx, path, name, value, size, flags);
+    return v9fs_set_xattr(fs_ctx, path, name, value, size, flags);
 }
 
-static int local_lremovexattr(FsContext *ctx,
+static int local_lremovexattr(FsContext *fs_ctx,
                               const char *path, const char *name)
 {
-    return v9fs_remove_xattr(ctx, path, name);
+    return v9fs_remove_xattr(fs_ctx, path, name);
 }
 
-static int local_syncfs(FsContext *ctx, int fd)
+static int local_syncfs(FsContext *fs_ctx, int fd)
 {
     /*
      * We should be doing per file system sync here.
diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
index 6b2e16a..3ea4f53 100644
--- a/hw/file-op-9p.h
+++ b/hw/file-op-9p.h
@@ -102,11 +102,11 @@  typedef struct FileOperations
     void *opaque;
 } FileOperations;
 
-static inline const char *rpath(FsContext *ctx, const char *path)
+static inline char *rpath(FsContext *ctx, const char *path)
 {
-    /* FIXME: so wrong... */
-    static char buffer[4096];
-    snprintf(buffer, sizeof(buffer), "%s/%s", ctx->fs_root, path);
-    return buffer;
+    char *fullpath;
+    fullpath = qemu_malloc(strlen(path) + strlen(ctx->fs_root) + 2);
+    sprintf(fullpath, "%s/%s", ctx->fs_root, path);
+    return fullpath;
 }
 #endif