Re: [Qemu-devel] [V2 06/25] [virtio-9p] coroutines for readlink

[Venkateswararao Jujjuri]

On 05/18/2011 02:43 AM, Stefan Hajnoczi wrote:
> On Tue, May 17, 2011 at 8:43 PM, Venkateswararao Jujjuri (JV)
> <address@hidden>  wrote:
> > Signed-off-by: Venkateswararao Jujjuri "<address@hidden>
> > ---
> >   Makefile.objs            |    2 +-
> >   hw/9pfs/cofs.c           |   42 ++++++++++++++++++++++++++++++++++++++++++
> >   hw/9pfs/virtio-9p-coth.h |    1 +
> >   hw/9pfs/virtio-9p.c      |   27 ++++-----------------------
> >   hw/9pfs/virtio-9p.h      |    3 ++-
> >   5 files changed, 50 insertions(+), 25 deletions(-)
> >   create mode 100644 hw/9pfs/cofs.c
> >
> > diff --git a/Makefile.objs b/Makefile.objs
> > index 96f6a24..36005bb 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -297,7 +297,7 @@ hw-obj-$(CONFIG_SOUND) += $(sound-obj-y)
> >   9pfs-nested-$(CONFIG_VIRTFS) = virtio-9p.o virtio-9p-debug.o
> >   9pfs-nested-$(CONFIG_VIRTFS) +=  virtio-9p-local.o virtio-9p-xattr.o
> >   9pfs-nested-$(CONFIG_VIRTFS) +=   virtio-9p-xattr-user.o virtio-9p-posix-acl.o
> > -9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-coth.o
> > +9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-coth.o cofs.o
> >
> >   hw-obj-$(CONFIG_REALLY_VIRTFS) += $(addprefix 9pfs/, $(9pfs-nested-y))
> >   $(addprefix 9pfs/, $(9pfs-nested-y)): QEMU_CFLAGS+=$(GLIB_CFLAGS)
> > diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
> > new file mode 100644
> > index 0000000..6d94673
> > --- /dev/null
> > +++ b/hw/9pfs/cofs.c
> > @@ -0,0 +1,42 @@
> > +
> > +/*
> > + * Virtio 9p backend
> > + *
> > + * Copyright IBM, Corp. 2011
> > + *
> > + * Authors:
> > + *  Aneesh Kumar K.V<address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > + * the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "fsdev/qemu-fsdev.h"
> > +#include "qemu-thread.h"
> > +#include "qemu-coroutine.h"
> > +#include "virtio-9p-coth.h"
> > +
> > +int v9fs_co_readlink(V9fsState *s, V9fsString *path, V9fsString *buf)
> > +{
> > +    int err;
> > +    ssize_t len;
> > +    V9fsString tbuf;
> > +
> > +    tbuf.data = qemu_malloc(PATH_MAX);
> Why introduce tbuf when the buf is available?  You end up having to
> copy back fields at the end of the function and load from an
> uninitialized address (tbuf.size) in the error case.
tbuf is introduced for re-entrent purpose.
We should be calling v9fs_string_init() on this though.

> > @@ -299,7 +282,7 @@ static void v9fs_string_init(V9fsString *str)
> >      str->size = 0;
> >   }
> >
> > -static void v9fs_string_free(V9fsString *str)
> > +void v9fs_string_free(V9fsString *str)
> >   {
> >      qemu_free(str->data);
> >      str->data = NULL;
> > @@ -421,7 +404,7 @@ v9fs_string_sprintf(V9fsString *str, const char *fmt, ...)
> >      str->size = err;
> >   }
> >
> > -static void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs)
> > +void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs)
> >   {
> >      v9fs_string_free(lhs);
> >      v9fs_string_sprintf(lhs, "%s", rhs->data);
> Spurious changes?  I don't see any users here.
Yep. Good catch.

> > @@ -1057,8 +1040,7 @@ static int stat_to_v9stat(V9fsState *s, V9fsString *name,
> >      v9fs_string_null(&v9stat->extension);
> >
> >      if (v9stat->mode&  P9_STAT_MODE_SYMLINK) {
> > -        ssize_t symlink_len;
> > -        err = v9fs_do_readlink(s, name,&v9stat->extension,&symlink_len);
> > +        err = v9fs_co_readlink(s, name,&v9stat->extension);
> >          if (err<  0) {
> >              return err;
> >          }
> > @@ -3591,7 +3573,6 @@ static void v9fs_readlink(void *opaque)
> >      int32_t fid;
> >      int err = 0;
> >      V9fsFidState *fidp;
> > -    ssize_t symlink_len;
> >
> >      pdu_unmarshal(pdu, offset, "d",&fid);
> >      fidp = lookup_fid(pdu->s, fid);
> > @@ -3601,7 +3582,7 @@ static void v9fs_readlink(void *opaque)
> >      }
> >
> >      v9fs_string_init(&target);
> > -    err = v9fs_do_readlink(pdu->s,&fidp->path,&target,&symlink_len);
> > +    err = v9fs_co_readlink(pdu->s,&fidp->path,&target);
> >      if (err<  0) {
> >          goto out;
> >      }
> Ah, the unused length argument from patch 5 has been removed.  It
> would be nice to do fixes earlier rather than later in the patch
> series, otherwise time is spent reviewing and commenting on code which
> gets removed.
The previous patch is just to move errno. So we thought of doing here while
we are introducing coroutines. But thinking now we could have done 
either place.

Thanks,
JV

> Stefan