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

[Stefan Hajnoczi]

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.

> @@ -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.

> @@ -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.

Stefan