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

[Venkateswararao Jujjuri]

On 05/18/2011 10:37 PM, Stefan Hajnoczi wrote:
> On Wed, May 18, 2011 at 7:42 PM, Venkateswararao Jujjuri
> <address@hidden>  wrote:
> > 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.
> I see no issue here and no safety added by using a local variable.
> Can you explain what you're trying to do?
No issues in the current usage. Just looking at the micro level of this 
routine,
we have an passd-in buffer passed under lock and is getting updated 
outside the lock (worker thread).
Can take it out, not an issue.

- JV

> Stefan