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

[Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>]

On 05/18/2011 02:43 AM, Stefan Hajnoczi wrote:
> On Tue, May 17, 2011 at 8:43 PM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com>  wrote:
>> Signed-off-by: Venkateswararao Jujjuri "<jvrao@linux.vnet.ibm.com>
>> ---
>>   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<aneesh.kumar@linux.vnet.ibm.com>
>> + *
>> + * 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
>