Patchwork [-V3,4/8] hw/9pfs: Implement syncfs

login
register
mail settings
Submitter Aneesh Kumar K.V
Date March 5, 2011, 5:52 p.m.
Message ID <1299347533-17047-4-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/85543/
State New
Headers show

Comments

Aneesh Kumar K.V - March 5, 2011, 5:52 p.m.
SYNOPSIS
    size[4] Tsyncfs tag[2] fid[4]

    size[4] Rsyncfs tag[2]

DESCRIPTION
    The Tsyncfs transaction transfers ("flushes") all modified data of
    file system identified by fid to the disk device. The operation is
    equivalent to calling sync() on the file system.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-local.c |    9 +++++++++
 hw/9pfs/virtio-9p.c       |   31 +++++++++++++++++++++++++++++++
 hw/9pfs/virtio-9p.h       |    2 ++
 hw/file-op-9p.h           |    1 +
 4 files changed, 43 insertions(+), 0 deletions(-)
Stefan Hajnoczi - March 13, 2011, 4:24 p.m.
On Sat, Mar 5, 2011 at 5:52 PM, Aneesh Kumar K.V
<aneesh.kumar@linux.vnet.ibm.com> wrote:
> +static void v9fs_post_do_syncfs(V9fsState *s, V9fsPDU *pdu, int err)
> +{
> +    if (err == -1) {
> +        err = -errno;
> +    }

errno may have been clobbered by any standard library function/syscall
invoked by this thread before v9fs_post_do_syncfs() was called.  Why
not pass the -errno in the function argument?

static void v9fs_post_do_syncfs(V9fsState *s, V9fsPDU *pdu, int err)
{
    complete_pdu(s, pdu, err);
}

I strongly suggest doing a separate patch series to clean up of all of
virtio-9p to stop using errno.  It's bad practice and I've caught
several errno mistakes in the virtio-9p patches I've reviewed - I bet
there are other instances lurking around.

> +static void v9fs_syncfs(V9fsState *s, V9fsPDU *pdu)
> +{
> +    int err;
> +    int32_t fid;
> +    size_t offset = 7;
> +    V9fsFidState *fidp;
> +
> +    pdu_unmarshal(pdu, offset, "d", &fid);
> +    fidp = lookup_fid(s, fid);
> +    if (fidp == NULL) {
> +        err = -ENOENT;
> +        v9fs_post_do_syncfs(s, pdu, err);
> +        return;
> +    }

This wasn't setting errno but passed the error in err.  It can stay
like this if you change v9fs_post_do_syncfs().

Stefan
Aneesh Kumar K.V - March 13, 2011, 6:59 p.m.
On Sun, 13 Mar 2011 16:24:13 +0000, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Sat, Mar 5, 2011 at 5:52 PM, Aneesh Kumar K.V
> <aneesh.kumar@linux.vnet.ibm.com> wrote:
> > +static void v9fs_post_do_syncfs(V9fsState *s, V9fsPDU *pdu, int err)
> > +{
> > +    if (err == -1) {
> > +        err = -errno;
> > +    }
> 
> errno may have been clobbered by any standard library function/syscall
> invoked by this thread before v9fs_post_do_syncfs() was called.  Why
> not pass the -errno in the function argument?
> 
> static void v9fs_post_do_syncfs(V9fsState *s, V9fsPDU *pdu, int err)
> {
>     complete_pdu(s, pdu, err);
> }
> 
> I strongly suggest doing a separate patch series to clean up of all of
> virtio-9p to stop using errno.  It's bad practice and I've caught
> several errno mistakes in the virtio-9p patches I've reviewed - I bet
> there are other instances lurking around.

Agreed.  I will look at the error handling more closely

> 
> > +static void v9fs_syncfs(V9fsState *s, V9fsPDU *pdu)
> > +{
> > +    int err;
> > +    int32_t fid;
> > +    size_t offset = 7;
> > +    V9fsFidState *fidp;
> > +
> > +    pdu_unmarshal(pdu, offset, "d", &fid);
> > +    fidp = lookup_fid(s, fid);
> > +    if (fidp == NULL) {
> > +        err = -ENOENT;
> > +        v9fs_post_do_syncfs(s, pdu, err);
> > +        return;
> > +    }
> 
> This wasn't setting errno but passed the error in err.  It can stay
> like this if you change v9fs_post_do_syncfs().
> 

nice catch. Will update.

-aneesh

Patch

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 0a015de..43ff37c 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -528,6 +528,14 @@  static int local_lremovexattr(FsContext *ctx,
     return v9fs_remove_xattr(ctx, path, name);
 }
 
+static int local_syncfs(FsContext *ctx, int fd)
+{
+    /*
+     * We should be doing per file system sync here.
+     */
+    sync();
+    return 0;
+}
 
 FileOperations local_ops = {
     .lstat = local_lstat,
@@ -560,4 +568,5 @@  FileOperations local_ops = {
     .llistxattr = local_llistxattr,
     .lsetxattr = local_lsetxattr,
     .lremovexattr = local_lremovexattr,
+    .syncfs     = local_syncfs,
 };
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index c4b0198..ce09e55 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -299,6 +299,10 @@  static int v9fs_do_lremovexattr(V9fsState *s, V9fsString *path,
                                 xattr_name->data);
 }
 
+static int v9fs_do_syncfs(V9fsState *s, int fd)
+{
+    return s->ops->syncfs(&s->ctx, fd);
+}
 
 static void v9fs_string_init(V9fsString *str)
 {
@@ -1978,6 +1982,32 @@  static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
     v9fs_post_do_fsync(s, pdu, err);
 }
 
+static void v9fs_post_do_syncfs(V9fsState *s, V9fsPDU *pdu, int err)
+{
+    if (err == -1) {
+        err = -errno;
+    }
+    complete_pdu(s, pdu, err);
+}
+
+static void v9fs_syncfs(V9fsState *s, V9fsPDU *pdu)
+{
+    int err;
+    int32_t fid;
+    size_t offset = 7;
+    V9fsFidState *fidp;
+
+    pdu_unmarshal(pdu, offset, "d", &fid);
+    fidp = lookup_fid(s, fid);
+    if (fidp == NULL) {
+        err = -ENOENT;
+        v9fs_post_do_syncfs(s, pdu, err);
+        return;
+    }
+    err = v9fs_do_syncfs(s, fidp->fsmap.fs.fd);
+    v9fs_post_do_syncfs(s, pdu, err);
+}
+
 static void v9fs_clunk(V9fsState *s, V9fsPDU *pdu)
 {
     int32_t fid;
@@ -3676,6 +3706,7 @@  static pdu_handler_t *pdu_handlers[] = {
     [P9_TWALK] = v9fs_walk,
     [P9_TCLUNK] = v9fs_clunk,
     [P9_TFSYNC] = v9fs_fsync,
+    [P9_TSYNCFS] = v9fs_syncfs,
     [P9_TOPEN] = v9fs_open,
     [P9_TREAD] = v9fs_read,
 #if 0
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 68d5906..b0f8210 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -13,6 +13,8 @@ 
 #define VIRTIO_9P_MOUNT_TAG 0
 
 enum {
+    P9_TSYNCFS = 0,
+    P9_RSYNCFS,
     P9_TLERROR = 6,
     P9_RLERROR,
     P9_TSTATFS = 8,
diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
index 126e60e..e306305 100644
--- a/hw/file-op-9p.h
+++ b/hw/file-op-9p.h
@@ -94,6 +94,7 @@  typedef struct FileOperations
     int (*lsetxattr)(FsContext *, const char *,
                      const char *, void *, size_t, int);
     int (*lremovexattr)(FsContext *, const char *, const char *);
+    int (*syncfs)(FsContext *, int);
     void *opaque;
 } FileOperations;