| Message ID | 1299347533-17047-4-git-send-email-aneesh.kumar@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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;
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(-)