Re: [Qemu-devel] idea: non-ethernet paravirtual network device

[Sassan Panahinejad]

Ok, thanks guys. I don't think I'll proceed with this idea.

Thanks
Sassan

On 26 July 2011 11:23, Hannes Reinecke h...@suse.de wrote:

 On 07/26/2011 08:04 AM, Stefan Hajnoczi wrote:

 On Mon, Jul 25, 2011 at 4:53 PM, Sassan 
 Panahinejadsas...@sassan.me.**uksas...@sassan.me.uk
  wrote:

 Here's a thought, could we improve network performance by creating a
 paravirtual network device which doesn't emulate ethernet? It shouldn't
 be
 too hard to just whack IP packets pretty much directly over a virtio
 link.
 This should improve performance when using a user host connection and
 we
 could introduce a tun host connection instead of tap for this setup.

 Does anyone have any thoughts on how worthwhile this would be? Would the
 performance improvement justify the effort involved?


 My guess is no noticable impact (if you ignore ARP requests).

 The Ethernet header is only 14 bytes or so.  We don't calculate any
 checksums at that level.  There's probably not much of a win.

  Only lots of pain to be had.
 Mainframe used to do this. But abandoned it not, thankfully.

 Problem is that you need to patch each and every tool looking at the
 packets to _not_ expecting an Ethernet header.
 And patching up DHCP is _not_ trivial.

 Cheers,

 Hannes
 --
 Dr. Hannes Reinecke   zSeries  Storage
 h...@suse.de  +49 911 74053 688
 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
 GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[Qemu-devel] idea: non-ethernet paravirtual network device

[Sassan Panahinejad]

Hi all,

Here's a thought, could we improve network performance by creating a
paravirtual network device which doesn't emulate ethernet? It shouldn't be
too hard to just whack IP packets pretty much directly over a virtio link.
This should improve performance when using a user host connection and we
could introduce a tun host connection instead of tap for this setup.

Does anyone have any thoughts on how worthwhile this would be? Would the
performance improvement justify the effort involved?

Thanks
Sassan

Re: [Qemu-devel] [PATCH] Clean up virtio-9p error handling code

[Sassan Panahinejad]

Hi JV,

Any progress regarding merging this patch (and the fsync patch I submitted)?
Is there anything I can do to assist/speed the process?

Thanks
Sassan


On 8 June 2011 17:21, Sassan Panahinejad sas...@sassan.me.uk wrote:

 In a lot of cases, the handling of errors was quite ugly.
 This patch moves reading of errno to immediately after the system calls and
 passes it up through the system more cleanly.
 Also, in the case of the xattr functions (and possibly others), completely
 the wrong error was being returned.


 This patch is created against your 9p-coroutine-bh branch, as requested.
 Sorry for the delay, I was unexpectedly required to work abroad for 2 weeks.

 Signed-off-by: Sassan Panahinejad sas...@sassan.me.uk

 ---
  fsdev/file-op-9p.h|4 +-
  hw/9pfs/codir.c   |   14 +
  hw/9pfs/virtio-9p-local.c |  123
 +
  hw/9pfs/virtio-9p-xattr.c |   21 +++-
  4 files changed, 90 insertions(+), 72 deletions(-)

 diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
 index af9daf7..3d9575b 100644
 --- a/fsdev/file-op-9p.h
 +++ b/fsdev/file-op-9p.h
 @@ -73,12 +73,12 @@ typedef struct FileOperations
 int (*setuid)(FsContext *, uid_t);
 int (*close)(FsContext *, int);
 int (*closedir)(FsContext *, DIR *);
 -DIR *(*opendir)(FsContext *, const char *);
 +int (*opendir)(FsContext *, const char *, DIR **);
 int (*open)(FsContext *, const char *, int);
 int (*open2)(FsContext *, const char *, int, FsCred *);
 void (*rewinddir)(FsContext *, DIR *);
 off_t (*telldir)(FsContext *, DIR *);
 -struct dirent *(*readdir)(FsContext *, DIR *);
 +int (*readdir)(FsContext *, DIR *, struct dirent **);
 void (*seekdir)(FsContext *, DIR *, off_t);
 ssize_t (*preadv)(FsContext *, int, const struct iovec *, int, off_t);
 ssize_t (*pwritev)(FsContext *, int, const struct iovec *, int, off_t);
 diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
 index 110289f..acbbb39 100644
 --- a/hw/9pfs/codir.c
 +++ b/hw/9pfs/codir.c
 @@ -25,12 +25,7 @@ int v9fs_co_readdir(V9fsState *s, V9fsFidState *fidp,
 struct dirent **dent)
 {
 errno = 0;
 /*FIXME!! need to switch to readdir_r */
 -*dent = s-ops-readdir(s-ctx, fidp-fs.dir);
 -if (!*dent  errno) {
 -err = -errno;
 -} else {
 -err = 0;
 -}
 +err = s-ops-readdir(s-ctx, fidp-fs.dir, dent);
 });
 return err;
  }
 @@ -93,12 +88,7 @@ int v9fs_co_opendir(V9fsState *s, V9fsFidState *fidp)

 v9fs_co_run_in_worker(
 {
 -dir = s-ops-opendir(s-ctx, fidp-path.data);
 -if (!dir) {
 -err = -errno;
 -} else {
 -err = 0;
 -}
 +err = s-ops-opendir(s-ctx, fidp-path.data, dir);
 });
 fidp-fs.dir = dir;
 return err;
 diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
 index 77904c3..65f35eb 100644
 --- a/hw/9pfs/virtio-9p-local.c
 +++ b/hw/9pfs/virtio-9p-local.c
 @@ -28,7 +28,7 @@ static int local_lstat(FsContext *fs_ctx, const char
 *path, struct stat *stbuf)
 char buffer[PATH_MAX];
 err =  lstat(rpath(fs_ctx, path, buffer), stbuf);
  if (err) {
 -return err;
 +return -errno;
 }
 if (fs_ctx-fs_sm == SM_MAPPED) {
 /* Actual credentials are part of extended attrs */
 @@ -53,7 +53,7 @@ static int local_lstat(FsContext *fs_ctx, const char
 *path, struct stat *stbuf)
  stbuf-st_rdev = tmp_dev;
 }
 }
 -return err;
 +return 0;
  }

  static int local_set_xattr(const char *path, FsCred *credp)
 @@ -63,28 +63,28 @@ static int local_set_xattr(const char *path, FsCred
 *credp)
  err = setxattr(path, user.virtfs.uid, credp-fc_uid,
 sizeof(uid_t),
 0);
 if (err) {
 -return err;
 +return -errno;
 }
 }
 if (credp-fc_gid != -1) {
 err = setxattr(path, user.virtfs.gid, credp-fc_gid,
 sizeof(gid_t),
 0);
 if (err) {
 -return err;
 +return -errno;
 }
 }
 if (credp-fc_mode != -1) {
 err = setxattr(path, user.virtfs.mode, credp-fc_mode,
 sizeof(mode_t), 0);
 if (err) {
 -return err;
 +return -errno;
 }
 }
 if (credp-fc_rdev != -1) {
 err = setxattr(path, user.virtfs.rdev, credp-fc_rdev,
 sizeof(dev_t), 0);
 if (err) {
 -return err;
 +return -errno;
 }
 }
 return 0;
 @@ -95,7 +95,7 @@ static int local_post_create_passthrough(FsContext
 *fs_ctx, const char *path,
  {
 char buffer[PATH_MAX];
 if (chmod(rpath(fs_ctx, path, buffer), credp-fc_mode  0)  0) {
 -return -1;
 +return -errno;
 }
 if (lchown(rpath(fs_ctx, path, buffer), credp-fc_uid

Re: [Qemu-devel] [PATCH] Clean up virtio-9p error handling code

[Sassan Panahinejad]

On 28 June 2011 15:22, Venkateswararao Jujjuri jv...@linux.vnet.ibm.comwrote:

 **
 On 06/28/2011 05:25 AM, Sassan Panahinejad wrote:

 Hi JV,

 Any progress regarding merging this patch (and the fsync patch I
 submitted)?
 Is there anything I can do to assist/speed the process?

 Sussan, Thanks for the ping. :)

 Everything is on hold waiting for 0.15 tag; Including threading/coroutine
 patches.
 As soon as we go in with the coroutines, you can just rebase your patch and
 we should be
 ready to go. I am waiting too. :-D

Thanks for the info :)


 Thanks,
 JV



 Thanks
 Sassan


 On 8 June 2011 17:21, Sassan Panahinejad sas...@sassan.me.uk wrote:

 In a lot of cases, the handling of errors was quite ugly.
 This patch moves reading of errno to immediately after the system calls
 and passes it up through the system more cleanly.
 Also, in the case of the xattr functions (and possibly others), completely
 the wrong error was being returned.


  This patch is created against your 9p-coroutine-bh branch, as requested.
 Sorry for the delay, I was unexpectedly required to work abroad for 2 weeks.

 Signed-off-by: Sassan Panahinejad sas...@sassan.me.uk

 ---
  fsdev/file-op-9p.h|4 +-
   hw/9pfs/codir.c   |   14 +
  hw/9pfs/virtio-9p-local.c |  123
 +
   hw/9pfs/virtio-9p-xattr.c |   21 +++-
  4 files changed, 90 insertions(+), 72 deletions(-)

 diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
  index af9daf7..3d9575b 100644
 --- a/fsdev/file-op-9p.h
 +++ b/fsdev/file-op-9p.h
 @@ -73,12 +73,12 @@ typedef struct FileOperations
 int (*setuid)(FsContext *, uid_t);
 int (*close)(FsContext *, int);
 int (*closedir)(FsContext *, DIR *);
 -DIR *(*opendir)(FsContext *, const char *);
 +int (*opendir)(FsContext *, const char *, DIR **);
 int (*open)(FsContext *, const char *, int);
 int (*open2)(FsContext *, const char *, int, FsCred *);
 void (*rewinddir)(FsContext *, DIR *);
 off_t (*telldir)(FsContext *, DIR *);
 -struct dirent *(*readdir)(FsContext *, DIR *);
 +int (*readdir)(FsContext *, DIR *, struct dirent **);
 void (*seekdir)(FsContext *, DIR *, off_t);
 ssize_t (*preadv)(FsContext *, int, const struct iovec *, int, off_t);
 ssize_t (*pwritev)(FsContext *, int, const struct iovec *, int,
 off_t);
  diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
 index 110289f..acbbb39 100644
 --- a/hw/9pfs/codir.c
 +++ b/hw/9pfs/codir.c
 @@ -25,12 +25,7 @@ int v9fs_co_readdir(V9fsState *s, V9fsFidState *fidp,
 struct dirent **dent)
 {
 errno = 0;
 /*FIXME!! need to switch to readdir_r */
 -*dent = s-ops-readdir(s-ctx, fidp-fs.dir);
 -if (!*dent  errno) {
 -err = -errno;
 -} else {
 -err = 0;
 -}
 +err = s-ops-readdir(s-ctx, fidp-fs.dir, dent);
 });
 return err;
  }
 @@ -93,12 +88,7 @@ int v9fs_co_opendir(V9fsState *s, V9fsFidState *fidp)

 v9fs_co_run_in_worker(
 {
 -dir = s-ops-opendir(s-ctx, fidp-path.data);
 -if (!dir) {
 -err = -errno;
 -} else {
 -err = 0;
 -}
 +err = s-ops-opendir(s-ctx, fidp-path.data, dir);
 });
 fidp-fs.dir = dir;
 return err;
 diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
  index 77904c3..65f35eb 100644
 --- a/hw/9pfs/virtio-9p-local.c
 +++ b/hw/9pfs/virtio-9p-local.c
  @@ -28,7 +28,7 @@ static int local_lstat(FsContext *fs_ctx, const char
 *path, struct stat *stbuf)
 char buffer[PATH_MAX];
 err =  lstat(rpath(fs_ctx, path, buffer), stbuf);
  if (err) {
 -return err;
 +return -errno;
 }
 if (fs_ctx-fs_sm == SM_MAPPED) {
 /* Actual credentials are part of extended attrs */
  @@ -53,7 +53,7 @@ static int local_lstat(FsContext *fs_ctx, const char
 *path, struct stat *stbuf)
  stbuf-st_rdev = tmp_dev;
 }
 }
 -return err;
 +return 0;
  }

  static int local_set_xattr(const char *path, FsCred *credp)
  @@ -63,28 +63,28 @@ static int local_set_xattr(const char *path, FsCred
 *credp)
  err = setxattr(path, user.virtfs.uid, credp-fc_uid,
 sizeof(uid_t),
 0);
 if (err) {
 -return err;
 +return -errno;
 }
 }
 if (credp-fc_gid != -1) {
 err = setxattr(path, user.virtfs.gid, credp-fc_gid,
 sizeof(gid_t),
 0);
 if (err) {
 -return err;
 +return -errno;
 }
 }
 if (credp-fc_mode != -1) {
 err = setxattr(path, user.virtfs.mode, credp-fc_mode,
 sizeof(mode_t), 0);
 if (err) {
 -return err;
 +return -errno;
 }
 }
 if (credp-fc_rdev != -1) {
 err = setxattr(path, user.virtfs.rdev, credp-fc_rdev,
 sizeof

[Qemu-devel] [PATCH] Clean up virtio-9p error handling code

[Sassan Panahinejad]

In a lot of cases, the handling of errors was quite ugly.
This patch moves reading of errno to immediately after the system calls and 
passes it up through the system more cleanly.
Also, in the case of the xattr functions (and possibly others), completely the 
wrong error was being returned.


This patch is created against your 9p-coroutine-bh branch, as requested. Sorry 
for the delay, I was unexpectedly required to work abroad for 2 weeks.

Signed-off-by: Sassan Panahinejad sas...@sassan.me.uk

---
 fsdev/file-op-9p.h|4 +-
 hw/9pfs/codir.c   |   14 +
 hw/9pfs/virtio-9p-local.c |  123 +
 hw/9pfs/virtio-9p-xattr.c |   21 +++-
 4 files changed, 90 insertions(+), 72 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index af9daf7..3d9575b 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -73,12 +73,12 @@ typedef struct FileOperations
 int (*setuid)(FsContext *, uid_t);
 int (*close)(FsContext *, int);
 int (*closedir)(FsContext *, DIR *);
-DIR *(*opendir)(FsContext *, const char *);
+int (*opendir)(FsContext *, const char *, DIR **);
 int (*open)(FsContext *, const char *, int);
 int (*open2)(FsContext *, const char *, int, FsCred *);
 void (*rewinddir)(FsContext *, DIR *);
 off_t (*telldir)(FsContext *, DIR *);
-struct dirent *(*readdir)(FsContext *, DIR *);
+int (*readdir)(FsContext *, DIR *, struct dirent **);
 void (*seekdir)(FsContext *, DIR *, off_t);
 ssize_t (*preadv)(FsContext *, int, const struct iovec *, int, off_t);
 ssize_t (*pwritev)(FsContext *, int, const struct iovec *, int, off_t);
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 110289f..acbbb39 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -25,12 +25,7 @@ int v9fs_co_readdir(V9fsState *s, V9fsFidState *fidp, struct 
dirent **dent)
 {
 errno = 0;
 /*FIXME!! need to switch to readdir_r */
-*dent = s-ops-readdir(s-ctx, fidp-fs.dir);
-if (!*dent  errno) {
-err = -errno;
-} else {
-err = 0;
-}
+err = s-ops-readdir(s-ctx, fidp-fs.dir, dent);
 });
 return err;
 }
@@ -93,12 +88,7 @@ int v9fs_co_opendir(V9fsState *s, V9fsFidState *fidp)
 
 v9fs_co_run_in_worker(
 {
-dir = s-ops-opendir(s-ctx, fidp-path.data);
-if (!dir) {
-err = -errno;
-} else {
-err = 0;
-}
+err = s-ops-opendir(s-ctx, fidp-path.data, dir);
 });
 fidp-fs.dir = dir;
 return err;
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 77904c3..65f35eb 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -28,7 +28,7 @@ static int local_lstat(FsContext *fs_ctx, const char *path, 
struct stat *stbuf)
 char buffer[PATH_MAX];
 err =  lstat(rpath(fs_ctx, path, buffer), stbuf);
 if (err) {
-return err;
+return -errno;
 }
 if (fs_ctx-fs_sm == SM_MAPPED) {
 /* Actual credentials are part of extended attrs */
@@ -53,7 +53,7 @@ static int local_lstat(FsContext *fs_ctx, const char *path, 
struct stat *stbuf)
 stbuf-st_rdev = tmp_dev;
 }
 }
-return err;
+return 0;
 }
 
 static int local_set_xattr(const char *path, FsCred *credp)
@@ -63,28 +63,28 @@ static int local_set_xattr(const char *path, FsCred *credp)
 err = setxattr(path, user.virtfs.uid, credp-fc_uid, sizeof(uid_t),
 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 if (credp-fc_gid != -1) {
 err = setxattr(path, user.virtfs.gid, credp-fc_gid, sizeof(gid_t),
 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 if (credp-fc_mode != -1) {
 err = setxattr(path, user.virtfs.mode, credp-fc_mode,
 sizeof(mode_t), 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 if (credp-fc_rdev != -1) {
 err = setxattr(path, user.virtfs.rdev, credp-fc_rdev,
 sizeof(dev_t), 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 return 0;
@@ -95,7 +95,7 @@ static int local_post_create_passthrough(FsContext *fs_ctx, 
const char *path,
 {
 char buffer[PATH_MAX];
 if (chmod(rpath(fs_ctx, path, buffer), credp-fc_mode  0)  0) {
-return -1;
+return -errno;
 }
 if (lchown(rpath(fs_ctx, path, buffer), credp-fc_uid,
 credp-fc_gid)  0) {
@@ -104,7 +104,7 @@ static int local_post_create_passthrough(FsContext *fs_ctx, 
const char *path,
  * using security model none. Ignore the error
  */
 if (fs_ctx-fs_sm != SM_NONE) {
-return -1;
+return -errno

[Qemu-devel] [PATCH] Clean up virtio-9p error handling code

[Sassan Panahinejad]

In a lot of cases, the handling of errors was quite ugly.
This patch moves reading of errno to immediately after the system calls and 
passes it up through the system more cleanly.
Also, in the case of the xattr functions (and possibly others), completely the 
wrong error was being returned.

Signed-off-by: Sassan Panahinejad sas...@sassan.me.uk
---
 fsdev/file-op-9p.h|4 +-
 hw/9pfs/virtio-9p-local.c |  123 +
 hw/9pfs/virtio-9p-xattr.c |   21 ++-
 hw/9pfs/virtio-9p.c   |  150 -
 4 files changed, 128 insertions(+), 170 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 126e60e..81a822a 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -73,12 +73,12 @@ typedef struct FileOperations
 int (*setuid)(FsContext *, uid_t);
 int (*close)(FsContext *, int);
 int (*closedir)(FsContext *, DIR *);
-DIR *(*opendir)(FsContext *, const char *);
+int (*opendir)(FsContext *, const char *, DIR **);
 int (*open)(FsContext *, const char *, int);
 int (*open2)(FsContext *, const char *, int, FsCred *);
 void (*rewinddir)(FsContext *, DIR *);
 off_t (*telldir)(FsContext *, DIR *);
-struct dirent *(*readdir)(FsContext *, DIR *);
+int (*readdir)(FsContext *, DIR *, struct dirent **);
 void (*seekdir)(FsContext *, DIR *, off_t);
 ssize_t (*preadv)(FsContext *, int, const struct iovec *, int, off_t);
 ssize_t (*pwritev)(FsContext *, int, const struct iovec *, int, off_t);
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 0a015de..de2597d 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -26,7 +26,7 @@ static int local_lstat(FsContext *fs_ctx, const char *path, 
struct stat *stbuf)
 int err;
 err =  lstat(rpath(fs_ctx, path), stbuf);
 if (err) {
-return err;
+return -errno;
 }
 if (fs_ctx-fs_sm == SM_MAPPED) {
 /* Actual credentials are part of extended attrs */
@@ -51,7 +51,7 @@ static int local_lstat(FsContext *fs_ctx, const char *path, 
struct stat *stbuf)
 stbuf-st_rdev = tmp_dev;
 }
 }
-return err;
+return 0;
 }
 
 static int local_set_xattr(const char *path, FsCred *credp)
@@ -61,28 +61,28 @@ static int local_set_xattr(const char *path, FsCred *credp)
 err = setxattr(path, user.virtfs.uid, credp-fc_uid, sizeof(uid_t),
 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 if (credp-fc_gid != -1) {
 err = setxattr(path, user.virtfs.gid, credp-fc_gid, sizeof(gid_t),
 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 if (credp-fc_mode != -1) {
 err = setxattr(path, user.virtfs.mode, credp-fc_mode,
 sizeof(mode_t), 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 if (credp-fc_rdev != -1) {
 err = setxattr(path, user.virtfs.rdev, credp-fc_rdev,
 sizeof(dev_t), 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 return 0;
@@ -92,7 +92,7 @@ static int local_post_create_passthrough(FsContext *fs_ctx, 
const char *path,
 FsCred *credp)
 {
 if (chmod(rpath(fs_ctx, path), credp-fc_mode  0)  0) {
-return -1;
+return -errno;
 }
 if (lchown(rpath(fs_ctx, path), credp-fc_uid, credp-fc_gid)  0) {
 /*
@@ -100,7 +100,7 @@ static int local_post_create_passthrough(FsContext *fs_ctx, 
const char *path,
  * using security model none. Ignore the error
  */
 if (fs_ctx-fs_sm != SM_NONE) {
-return -1;
+return -errno;
 }
 }
 return 0;
@@ -114,38 +114,40 @@ static ssize_t local_readlink(FsContext *fs_ctx, const 
char *path,
 int fd;
 fd = open(rpath(fs_ctx, path), O_RDONLY);
 if (fd == -1) {
-return -1;
+return -errno;
 }
 do {
 tsize = read(fd, (void *)buf, bufsz);
 } while (tsize == -1  errno == EINTR);
 close(fd);
-return tsize;
+return tsize == -1 ? -errno : tsize;
 } else if ((fs_ctx-fs_sm == SM_PASSTHROUGH) ||
(fs_ctx-fs_sm == SM_NONE)) {
 tsize = readlink(rpath(fs_ctx, path), buf, bufsz);
 }
-return tsize;
+return tsize == -1 ? -errno : tsize;
 }
 
 static int local_close(FsContext *ctx, int fd)
 {
-return close(fd);
+return close(fd) == -1 ? -errno : 0;
 }
 
 static int local_closedir(FsContext *ctx, DIR *dir)
 {
-return closedir(dir);
+return closedir(dir) == -1 ? -errno : 0;
 }
 
 static int local_open(FsContext *ctx, const char *path, int flags)
 {
-return open(rpath(ctx, path), flags);
+int ret = open(rpath(ctx, path), flags);
+return ret == -1 ? -errno : ret

[Qemu-devel] [PATCH] Clean up virtio-9p error handling code

[Sassan Panahinejad]

In a lot of cases, the handling of errors was quite ugly.
This patch moves reading of errno to immediately after the system calls and 
passes it up through the system more cleanly.
Also, in the case of the xattr functions (and possibly others), completely the 
wrong error was being returned.
---
 fsdev/file-op-9p.h|4 +-
 hw/9pfs/virtio-9p-local.c |  121 
 hw/9pfs/virtio-9p-xattr.c |   21 ++-
 hw/9pfs/virtio-9p.c   |  150 +++-
 4 files changed, 125 insertions(+), 171 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 126e60e..81a822a 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -73,12 +73,12 @@ typedef struct FileOperations
 int (*setuid)(FsContext *, uid_t);
 int (*close)(FsContext *, int);
 int (*closedir)(FsContext *, DIR *);
-DIR *(*opendir)(FsContext *, const char *);
+int (*opendir)(FsContext *, const char *, DIR **);
 int (*open)(FsContext *, const char *, int);
 int (*open2)(FsContext *, const char *, int, FsCred *);
 void (*rewinddir)(FsContext *, DIR *);
 off_t (*telldir)(FsContext *, DIR *);
-struct dirent *(*readdir)(FsContext *, DIR *);
+int (*readdir)(FsContext *, DIR *, struct dirent **);
 void (*seekdir)(FsContext *, DIR *, off_t);
 ssize_t (*preadv)(FsContext *, int, const struct iovec *, int, off_t);
 ssize_t (*pwritev)(FsContext *, int, const struct iovec *, int, off_t);
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 0a015de..afaff78 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -26,7 +26,7 @@ static int local_lstat(FsContext *fs_ctx, const char *path, 
struct stat *stbuf)
 int err;
 err =  lstat(rpath(fs_ctx, path), stbuf);
 if (err) {
-return err;
+return -errno;
 }
 if (fs_ctx-fs_sm == SM_MAPPED) {
 /* Actual credentials are part of extended attrs */
@@ -51,7 +51,7 @@ static int local_lstat(FsContext *fs_ctx, const char *path, 
struct stat *stbuf)
 stbuf-st_rdev = tmp_dev;
 }
 }
-return err;
+return 0;
 }
 
 static int local_set_xattr(const char *path, FsCred *credp)
@@ -61,28 +61,28 @@ static int local_set_xattr(const char *path, FsCred *credp)
 err = setxattr(path, user.virtfs.uid, credp-fc_uid, sizeof(uid_t),
 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 if (credp-fc_gid != -1) {
 err = setxattr(path, user.virtfs.gid, credp-fc_gid, sizeof(gid_t),
 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 if (credp-fc_mode != -1) {
 err = setxattr(path, user.virtfs.mode, credp-fc_mode,
 sizeof(mode_t), 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 if (credp-fc_rdev != -1) {
 err = setxattr(path, user.virtfs.rdev, credp-fc_rdev,
 sizeof(dev_t), 0);
 if (err) {
-return err;
+return -errno;
 }
 }
 return 0;
@@ -92,7 +92,7 @@ static int local_post_create_passthrough(FsContext *fs_ctx, 
const char *path,
 FsCred *credp)
 {
 if (chmod(rpath(fs_ctx, path), credp-fc_mode  0)  0) {
-return -1;
+return -errno;
 }
 if (lchown(rpath(fs_ctx, path), credp-fc_uid, credp-fc_gid)  0) {
 /*
@@ -100,7 +100,7 @@ static int local_post_create_passthrough(FsContext *fs_ctx, 
const char *path,
  * using security model none. Ignore the error
  */
 if (fs_ctx-fs_sm != SM_NONE) {
-return -1;
+return -errno;
 }
 }
 return 0;
@@ -114,38 +114,40 @@ static ssize_t local_readlink(FsContext *fs_ctx, const 
char *path,
 int fd;
 fd = open(rpath(fs_ctx, path), O_RDONLY);
 if (fd == -1) {
-return -1;
+return -errno;
 }
 do {
 tsize = read(fd, (void *)buf, bufsz);
 } while (tsize == -1  errno == EINTR);
 close(fd);
-return tsize;
+return tsize = 0 ? tsize : -errno;
 } else if ((fs_ctx-fs_sm == SM_PASSTHROUGH) ||
(fs_ctx-fs_sm == SM_NONE)) {
 tsize = readlink(rpath(fs_ctx, path), buf, bufsz);
 }
-return tsize;
+return tsize = 0 ? tsize : -errno;
 }
 
 static int local_close(FsContext *ctx, int fd)
 {
-return close(fd);
+return close(fd) ? -errno : 0;
 }
 
 static int local_closedir(FsContext *ctx, DIR *dir)
 {
-return closedir(dir);
+return closedir(dir) ? -errno : 0;
 }
 
 static int local_open(FsContext *ctx, const char *path, int flags)
 {
-return open(rpath(ctx, path), flags);
+int ret = open(rpath(ctx, path), flags);
+return ret = 0 ? ret : -errno;
 }
 
-static DIR *local_opendir(FsContext *ctx, const char *path)
+static int 

Re: [Qemu-devel] Bug in virtio-9p when fstatting an fd referring to a file that no longer exists

[Sassan Panahinejad]

On 2 May 2011 16:47, Venkateswararao Jujjuri jv...@linux.vnet.ibm.comwrote:

  On 04/28/2011 09:51 AM, Sassan Panahinejad wrote:

 This thread seems relevant:
 http://www.mail-archive.com/linux-fsdevel@vger.kernel.org/msg09159.html
 Unless things have changed, it looks like the problem is in the client
 kernel (although note that there isn't support in qemu, even if the client
 did send an fid associated with an open file!).
 Any thoughts on a workaround for this?


 Hrm, I don't see any workaround for this. May be we should add TFSTAT for
 dotl? or add a flag to
 TSTAT?


It's definitely a tricky problem. I can't think of how to go about dealing
with it.
Probably worth looking into how nfs deals with it (and also looking into
what happens when communicating between two native plan9 systems). I'll
start reading.

Thanks
Sassan



 Copying the v9fs.

 Thanks,
 JV





 Thanks
 Sassan

 On 28 April 2011 17:13, Sassan Panahinejad sas...@sassan.me.uk wrote:

 It should be possible for guest applications to fstat a file for which
 they have a valid file descriptor, even if the file has been removed.
 Demonstrated by the code sample below (fstat reports no such file or
 directory).
 Strangely it seems that reading from a file in this state works fine (and
 when both are run, the server receives a different fid for each).
 On any other filesystem, the code runs correctly. On our 9p filesystem it
 fails.
 Many applications (including bash) depend on this working correctly.
 I will continue investigating, but any thoughts anyone has on the subject
 would be appreciated.


 Thanks
 Sassan


 #include stdio.h
 #include unistd.h
 #include fcntl.h
 #include sys/types.h
 #include sys/stat.h


 int main(void)
 {
 int ret;
 struct stat statbuf;
 int fd = open(test.txt, O_RDWR | O_CREAT, 0666);
 if (fd  0) {
 printf(open failed: %m\n);
 return 1;
 }
 ret = write(fd, test1\n, 6);
 if (ret  0) {
 printf(write1 failed: %m\n);
 return 1;
 }
 ret = unlink(test.txt);
 if (ret  0) {
 printf(unlink failed: %m\n);
 return 1;
 }
 ret = fstat(fd, statbuf);
 if (ret  0) {
 printf(fstat failed: %m\n);
 return 1;
 }
 return 0;
 }

Re: [Qemu-devel] [PATCH] Fix bug with virtio-9p rename

[Sassan Panahinejad]

Malahal Naineni's patch Stop renaming files with similar name! (posted
since this one) also fixes this bug.
I don't mind which one gets merged, as long as the bug gets fixed ;)

Sassan


On 27 April 2011 19:30, Sassan Panahinejad sas...@sassan.me.uk wrote:

 After renaming a file, any existing references to the file are updated.
 However, in addition to this, it would update any files whos names began
 with that of the file being moved.
 Therefore when renaming somefile.txt to somefile.txt-old, any references to
 somefile.txt-new became somefile.txt-old-new.
 This breaks debconf and probably many other applications.
 This patch fixes the problem. Now only files exactly matching, or files
 which are a subdirectory of a directory being moved are affected.

 Signed-off-by: Sassan Panahinejad sas...@sassan.me.uk
 ---
  hw/virtio-9p.c |9 -
  1 files changed, 8 insertions(+), 1 deletions(-)

 diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
 index 2530f6d..a2f096d 100644
 --- a/hw/virtio-9p.c
 +++ b/hw/virtio-9p.c
 @@ -2810,8 +2810,15 @@ static int v9fs_complete_rename(V9fsState *s,
 V9fsRenameState *vs)
 */
 continue;
 }
 +/*
 +* Fix the name if
 +* A: The file is the one we're moving
 +* Or B: The file is a subdirectory of one we're moving
 +*/
 if (!strncmp(vs-fidp-path.data, fidp-path.data,
 -strlen(vs-fidp-path.data))) {
 +strlen(vs-fidp-path.data)) 
 +(strlen(vs-fidp-path.data) ==
 strlen(fidp-path.data) ||
 +fidp-path.data[strlen(vs-fidp-path.data)] == '/'))
 {
 /* replace the name */
 v9fs_fix_path(fidp-path, vs-name,
   strlen(vs-fidp-path.data));
 --
 1.7.0.4

[Qemu-devel] [PATCH] Fix bug with virtio-9p xattr functions return values

[Sassan Panahinejad]

Several functions in virtio-9p-xattr.c are passing the return value of the 
associated host system call back to the client instead of errno.
Since this value is -1 for any error (which, when received in the guest app as 
errno, indicates operation not permitted) it is causing guest applications to 
fail in cases where the operation is not supported by the host.
This causes a number of problems with dpkg and with install.
This patch fixes the bug and returns the correct value, which means that guest 
applications are able to handle the error correctly.

Signed-off-by: Sassan Panahinejad sas...@sassan.me.uk
---
 hw/virtio-9p-xattr.c |   21 ++---
 1 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-9p-xattr.c b/hw/virtio-9p-xattr.c
index 1aab081..fd6d892 100644
--- a/hw/virtio-9p-xattr.c
+++ b/hw/virtio-9p-xattr.c
@@ -32,9 +32,14 @@ static XattrOperations *get_xattr_operations(XattrOperations 
**h,
 ssize_t v9fs_get_xattr(FsContext *ctx, const char *path,
const char *name, void *value, size_t size)
 {
+int ret;
 XattrOperations *xops = get_xattr_operations(ctx-xops, name);
 if (xops) {
-return xops-getxattr(ctx, path, name, value, size);
+ret = xops-getxattr(ctx, path, name, value, size);
+if (ret  0) {
+return -errno;
+}
+return ret;
 }
 errno = -EOPNOTSUPP;
 return -1;
@@ -117,9 +122,14 @@ err_out:
 int v9fs_set_xattr(FsContext *ctx, const char *path, const char *name,
void *value, size_t size, int flags)
 {
+int ret;
 XattrOperations *xops = get_xattr_operations(ctx-xops, name);
 if (xops) {
-return xops-setxattr(ctx, path, name, value, size, flags);
+ret = xops-setxattr(ctx, path, name, value, size, flags);
+if (ret  0) {
+return -errno;
+}
+return ret;
 }
 errno = -EOPNOTSUPP;
 return -1;
@@ -129,9 +139,14 @@ int v9fs_set_xattr(FsContext *ctx, const char *path, const 
char *name,
 int v9fs_remove_xattr(FsContext *ctx,
   const char *path, const char *name)
 {
+int ret;
 XattrOperations *xops = get_xattr_operations(ctx-xops, name);
 if (xops) {
-return xops-removexattr(ctx, path, name);
+ret = xops-removexattr(ctx, path, name);
+if (ret  0) {
+return -errno;
+}
+return ret;
 }
 errno = -EOPNOTSUPP;
 return -1;
-- 
1.7.0.4

[Qemu-devel] Bug in virtio-9p when fstatting an fd referring to a file that no longer exists

[Sassan Panahinejad]

It should be possible for guest applications to fstat a file for which they
have a valid file descriptor, even if the file has been removed.
Demonstrated by the code sample below (fstat reports no such file or
directory).
Strangely it seems that reading from a file in this state works fine (and
when both are run, the server receives a different fid for each).
On any other filesystem, the code runs correctly. On our 9p filesystem it
fails.
Many applications (including bash) depend on this working correctly.
I will continue investigating, but any thoughts anyone has on the subject
would be appreciated.


Thanks
Sassan


#include stdio.h
#include unistd.h
#include fcntl.h
#include sys/types.h
#include sys/stat.h


int main(void)
{
int ret;
struct stat statbuf;
int fd = open(test.txt, O_RDWR | O_CREAT, 0666);
if (fd  0) {
printf(open failed: %m\n);
return 1;
}
ret = write(fd, test1\n, 6);
if (ret  0) {
printf(write1 failed: %m\n);
return 1;
}
ret = unlink(test.txt);
if (ret  0) {
printf(unlink failed: %m\n);
return 1;
}
ret = fstat(fd, statbuf);
if (ret  0) {
printf(fstat failed: %m\n);
return 1;
}
return 0;
}

Re: [Qemu-devel] Bug in virtio-9p when fstatting an fd referring to a file that no longer exists

[Sassan Panahinejad]

This thread seems relevant:
http://www.mail-archive.com/linux-fsdevel@vger.kernel.org/msg09159.html
Unless things have changed, it looks like the problem is in the client
kernel (although note that there isn't support in qemu, even if the client
did send an fid associated with an open file!).
Any thoughts on a workaround for this?


Thanks
Sassan

On 28 April 2011 17:13, Sassan Panahinejad sas...@sassan.me.uk wrote:

 It should be possible for guest applications to fstat a file for which they
 have a valid file descriptor, even if the file has been removed.
 Demonstrated by the code sample below (fstat reports no such file or
 directory).
 Strangely it seems that reading from a file in this state works fine (and
 when both are run, the server receives a different fid for each).
 On any other filesystem, the code runs correctly. On our 9p filesystem it
 fails.
 Many applications (including bash) depend on this working correctly.
 I will continue investigating, but any thoughts anyone has on the subject
 would be appreciated.


 Thanks
 Sassan


 #include stdio.h
 #include unistd.h
 #include fcntl.h
 #include sys/types.h
 #include sys/stat.h


 int main(void)
 {
 int ret;
 struct stat statbuf;
 int fd = open(test.txt, O_RDWR | O_CREAT, 0666);
 if (fd  0) {
 printf(open failed: %m\n);
 return 1;
 }
 ret = write(fd, test1\n, 6);
 if (ret  0) {
 printf(write1 failed: %m\n);
 return 1;
 }
 ret = unlink(test.txt);
 if (ret  0) {
 printf(unlink failed: %m\n);
 return 1;
 }
 ret = fstat(fd, statbuf);
 if (ret  0) {
 printf(fstat failed: %m\n);
 return 1;
 }
 return 0;
 }

[Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Sassan Panahinejad]

JV: I hope I have correctly understood your description of how lock/getlock 
should behave.

v9fs_fsync and possibly others break when asked to operate on a directory.
It does not check fid_type to see if it is operating on a directory and 
therefore accesses the wrong element of the fs union.
This error can result in guest applications failing (in my case it was dpkg).
This patch fixes the issue and implements correct behaviour for fsync, wstat, 
lock and getlock.

Signed-off-by: Sassan Panahinejad sas...@sassan.me.uk
---
 hw/virtio-9p.c |   36 
 1 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7e29535..2530f6d 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1875,7 +1875,13 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
 v9fs_post_do_fsync(s, pdu, err);
 return;
 }
-err = v9fs_do_fsync(s, fidp-fs.fd, datasync);
+if (fidp-fid_type == P9_FID_FILE) {
+err = v9fs_do_fsync(s, fidp-fs.fd, datasync);
+} else if (fidp-fid_type == P9_FID_DIR) {
+err = v9fs_do_fsync(s, dirfd(fidp-fs.dir), datasync);
+} else {
+err = -EINVAL;
+}
 v9fs_post_do_fsync(s, pdu, err);
 }
 
@@ -2999,7 +3005,13 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
 
 /* do we need to sync the file? */
 if (donttouch_stat(vs-v9stat)) {
-err = v9fs_do_fsync(s, vs-fidp-fs.fd, 0);
+if (vs-fidp-fid_type == P9_FID_FILE) {
+err = v9fs_do_fsync(s, vs-fidp-fs.fd, 0);
+} else if (vs-fidp-fid_type == P9_FID_DIR) {
+err = v9fs_do_fsync(s, dirfd(vs-fidp-fs.dir), 0);
+} else {
+err = -EINVAL;
+}
 v9fs_wstat_post_fsync(s, vs, err);
 return;
 }
@@ -3199,7 +3211,15 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu)
 goto out;
 }
 
-err = v9fs_do_fstat(s, vs-fidp-fs.fd, vs-stbuf);
+if (vs-fidp-fid_type == P9_FID_FILE) {
+err = v9fs_do_fstat(s, vs-fidp-fs.fd, vs-stbuf);
+} else if (vs-fidp-fid_type == P9_FID_DIR) {
+err = -EISDIR;
+goto out;
+} else {
+err = -EINVAL;
+goto out;
+}
 if (err  0) {
 err = -errno;
 goto out;
@@ -3237,7 +3257,15 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu)
 goto out;
 }
 
-err = v9fs_do_fstat(s, vs-fidp-fs.fd, vs-stbuf);
+if (vs-fidp-fid_type == P9_FID_FILE) {
+err = v9fs_do_fstat(s, vs-fidp-fs.fd, vs-stbuf);
+} else if (vs-fidp-fid_type == P9_FID_DIR) {
+err = -EISDIR;
+goto out;
+} else {
+err = -EINVAL;
+goto out;
+}
 if (err  0) {
 err = -errno;
 goto out;
-- 
1.7.0.4

[Qemu-devel] [PATCH] Fix bug with virtio-9p rename

[Sassan Panahinejad]

After renaming a file, any existing references to the file are updated.
However, in addition to this, it would update any files whos names began with 
that of the file being moved.
Therefore when renaming somefile.txt to somefile.txt-old, any references to 
somefile.txt-new became somefile.txt-old-new.
This breaks debconf and probably many other applications.
This patch fixes the problem. Now only files exactly matching, or files which 
are a subdirectory of a directory being moved are affected.

Signed-off-by: Sassan Panahinejad sas...@sassan.me.uk
---
 hw/virtio-9p.c |9 -
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 2530f6d..a2f096d 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -2810,8 +2810,15 @@ static int v9fs_complete_rename(V9fsState *s, 
V9fsRenameState *vs)
 */
 continue;
 }
+/*
+* Fix the name if
+* A: The file is the one we're moving
+* Or B: The file is a subdirectory of one we're moving
+*/
 if (!strncmp(vs-fidp-path.data, fidp-path.data,
-strlen(vs-fidp-path.data))) {
+strlen(vs-fidp-path.data)) 
+(strlen(vs-fidp-path.data) == strlen(fidp-path.data) ||
+fidp-path.data[strlen(vs-fidp-path.data)] == '/')) {
 /* replace the name */
 v9fs_fix_path(fidp-path, vs-name,
   strlen(vs-fidp-path.data));
-- 
1.7.0.4

[Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Sassan Panahinejad]

v9fs_fsync and possibly others break when asked to operate on a directory.
It does not check fid_type to see if it is operating on a directory and 
therefore accesses the wrong element of the fs union.
This error can result in guest applications failing (in my case it was dpkg).
This patch fixes the issue, although there may be other, similar bugs in 
virtio-9p.

Signed-off-by: Sassan Panahinejad sas...@sassan.me.uk
---
 hw/virtio-9p.c |6 +-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7e29535..cc4fdc8 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1875,7 +1875,11 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
 v9fs_post_do_fsync(s, pdu, err);
 return;
 }
-err = v9fs_do_fsync(s, fidp-fs.fd, datasync);
+if (fidp-fid_type == P9_FID_DIR) {
+err = v9fs_do_fsync(s, dirfd(fidp-fs.dir), datasync);
+} else {
+err = v9fs_do_fsync(s, fidp-fs.fd, datasync);
+}
 v9fs_post_do_fsync(s, pdu, err);
 }
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Sassan Panahinejad]

On 26 April 2011 13:58, Stefan Hajnoczi stefa...@gmail.com wrote:

 What about P9_FID_XATTR, seems like we have the same issue there too?

 wstat, lock, and getlock need closer auditing and perhaps fixing.

 Stefan


Sorry, forgot to hit reply-to-all.

Yes, it is probable that those functions will suffer from the same bug.
I will have to study XATTR and see how that will be affected. I don't know
whether it is possible for these functions to be called for XATTR, and if it
is then I do not know the proper way to handle it.
Perhaps we should have some function or macro to obtain the correct FD from
an fidp structure, which could be used for fsync, wstat, lock and getlock?

Sassan

[Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Sassan Panahinejad]

v9fs_fsync and possibly others break when asked to operate on a directory.
It does not check fid_type to see if it is operating on a directory and 
therefore accesses the wrong element of the fs union.
This error can result in guest applications failing (in my case it was dpkg).
This patch fixes the issue, although there may be other, similar bugs in 
virtio-9p.

Signed-off-by: Sassan Panahinejad sas...@sassan.me.uk
---

Here I've implemented it as a function. If you'd prefer a macro or inline 
function then let me know.

Thanks
Sassan

 hw/virtio-9p.c |   43 +--
 1 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7e29535..26be0fc 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1860,6 +1860,18 @@ static void v9fs_post_do_fsync(V9fsState *s, V9fsPDU 
*pdu, int err)
 complete_pdu(s, pdu, err);
 }
 
+static int v9fs_get_fd(V9fsFidState *fidp) 
+{
+switch (fidp-fid_type) {
+case P9_FID_FILE: 
+return fidp-fs.fd;
+case P9_FID_DIR: 
+return dirfd(fidp-fs.dir);
+default:
+return -1;
+}
+}
+
 static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
 {
 int32_t fid;
@@ -1867,6 +1879,7 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
 V9fsFidState *fidp;
 int datasync;
 int err;
+int fd;
 
 pdu_unmarshal(pdu, offset, dd, fid, datasync);
 fidp = lookup_fid(s, fid);
@@ -1875,7 +1888,11 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
 v9fs_post_do_fsync(s, pdu, err);
 return;
 }
-err = v9fs_do_fsync(s, fidp-fs.fd, datasync);
+if ((fd = v9fs_get_fd(fidp)) = 0) {
+err = v9fs_do_fsync(s, fd, datasync);
+} else {
+   err = -EINVAL;
+}
 v9fs_post_do_fsync(s, pdu, err);
 }
 
@@ -2984,6 +3001,7 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
 int32_t fid;
 V9fsWstatState *vs;
 int err = 0;
+int fd;
 
 vs = qemu_malloc(sizeof(*vs));
 vs-pdu = pdu;
@@ -2999,7 +3017,10 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
 
 /* do we need to sync the file? */
 if (donttouch_stat(vs-v9stat)) {
-err = v9fs_do_fsync(s, vs-fidp-fs.fd, 0);
+if ((fd = v9fs_get_fd(vs-fidp)) = 0) {
+err = v9fs_do_fsync(s, fd, 0);
+} else
+err = -EINVAL;
 v9fs_wstat_post_fsync(s, vs, err);
 return;
 }
@@ -3176,6 +3197,7 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu)
 {
 int32_t fid, err = 0;
 V9fsLockState *vs;
+int fd;
 
 vs = qemu_mallocz(sizeof(*vs));
 vs-pdu = pdu;
@@ -3198,8 +3220,12 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu)
 err = -ENOENT;
 goto out;
 }
-
-err = v9fs_do_fstat(s, vs-fidp-fs.fd, vs-stbuf);
+if ((fd = v9fs_get_fd(vs-fidp)) = 0) {
+err = v9fs_do_fstat(s, fd, vs-stbuf);
+} else {
+err = -EINVAL;
+goto out;
+}
 if (err  0) {
 err = -errno;
 goto out;
@@ -3221,6 +3247,7 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu)
 {
 int32_t fid, err = 0;
 V9fsGetlockState *vs;
+int fd;
 
 vs = qemu_mallocz(sizeof(*vs));
 vs-pdu = pdu;
@@ -3236,8 +3263,12 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu)
 err = -ENOENT;
 goto out;
 }
-
-err = v9fs_do_fstat(s, vs-fidp-fs.fd, vs-stbuf);
+if ((fd = v9fs_get_fd(vs-fidp)) = 0) {
+err = v9fs_do_fstat(s, fd, vs-stbuf);
+} else {
+err = -EINVAL;
+goto out;
+}
 if (err  0) {
 err = -errno;
 goto out;
-- 
1.7.0.4

[Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Sassan Panahinejad]

v9fs_fsync and possibly others break when asked to operate on a directory.
It does not check fid_type to see if it is operating on a directory and 
therefore accesses the wrong element of the fs union.
This error can result in guest applications failing (in my case it was dpkg).
This patch fixes the issue, although there may be other, similar bugs in 
virtio-9p.
---
 hw/virtio-9p.c |5 -
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7e29535..09fb5da 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1875,7 +1875,10 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
 v9fs_post_do_fsync(s, pdu, err);
 return;
 }
-err = v9fs_do_fsync(s, fidp-fs.fd, datasync);
+if (fidp-fid_type == P9_FID_DIR)
+err = v9fs_do_fsync(s, dirfd(fidp-fs.dir), datasync);
+else
+err = v9fs_do_fsync(s, fidp-fs.fd, datasync);
 v9fs_post_do_fsync(s, pdu, err);
 }
 
-- 
1.7.0.4