Re: [Qemu-devel] [PATCH 3/3] hw/9pfs: use g_strdup_printf() instead of PATH_MAX limitation

[Chen Gang <gang.chen.5i5j@gmail.com>]

On 03/04/2014 03:29 AM, Aneesh Kumar K.V wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
> 
>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>> Can we keep this as 
>>       v9fs_co_run_in_worker(
>>         {
>>                       
>>                buf->data = __readlink(&s->ctx, path);
>>
>>         }
>>
>> I can do that change for you if you want. I will also have to go through
>> the rest of the code to make sure we do free the memory in all error
>> path. So the rest of the review is going to take time. Hope that is ok
>>
> 
> I ended up with the below diff on top. The resulting tree is pushed to
> 
> https://github.com/kvaneesh/qemu/commits/for-upstream
> and pass all the tuxera tests. Let me know what you think.
> 

The 2 improvement (after the patch 3/3) looks fine to me, thanks.

And I guess, I need not send patch v2, again (if I still should/need
send patch v2 for it, please let me know, thanks).


> diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
> index 739bad019e3a..42ee614e27f0 100644
> --- a/hw/9pfs/cofs.c
> +++ b/hw/9pfs/cofs.c
> @@ -17,42 +17,55 @@
>  #include "block/coroutine.h"
>  #include "virtio-9p-coth.h"
>  
> +static ssize_t __readlink(V9fsState *s, V9fsPath *path, V9fsString *buf)
> +{
> +    ssize_t len, maxlen = PATH_MAX;
> +
> +    buf->data = g_malloc(PATH_MAX);
> +    for(;;) {
> +        len = s->ops->readlink(&s->ctx, path, buf->data, maxlen);
> +        if (len < 0) {
> +            g_free(buf->data);
> +            buf->data = NULL;
> +            buf->size = 0;
> +            break;
> +        } else if (len == maxlen) {
> +            /*
> +             * We dodn't have space to put the NULL or we have more
> +             * to read. Increase the size and try again
> +             */
> +            maxlen *= 2;
> +            g_free(buf->data);
> +            buf->data = g_malloc(maxlen);
> +            continue;
> +        }
> +        /*
> +         * Null terminate the readlink output
> +         */
> +        buf->data[len] = '\0';
> +        buf->size = len;
> +        break;
> +    }
> +    return len;
> +}
> +
>  int v9fs_co_readlink(V9fsPDU *pdu, V9fsPath *path, V9fsString *buf)
>  {
>      int err;
> -    ssize_t len, maxlen = PATH_MAX;
>      V9fsState *s = pdu->s;
>  
>      if (v9fs_request_cancelled(pdu)) {
>          return -EINTR;
>      }
> -    buf->data = g_malloc(maxlen);
>      v9fs_path_read_lock(s);
>      v9fs_co_run_in_worker(
> -        while (1) {
> -            len = s->ops->readlink(&s->ctx, path,
> -                                   buf->data, maxlen - 1);
> -            if (len == maxlen - 1) {
> -                g_free(buf->data);
> -                maxlen *= 2;
> -                buf->data = g_malloc(maxlen);
> -                continue;
> -            }
> -            if (len > -1) {
> -                buf->size = len;
> -                buf->data[len] = 0;
> -                err = 0;
> -            } else {
> +        {
> +            err = __readlink(s, path, buf);
> +            if (err < 0) {
>                  err = -errno;
>              }
> -            break;
>          });
>      v9fs_path_unlock(s);
> -    if (err) {
> -        g_free(buf->data);
> -        buf->data = NULL;
> -        buf->size = 0;
> -    }
>      return err;
>  }
>  
> diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
> index 9e9cc319ec54..56b302c122b6 100644
> --- a/hw/9pfs/virtio-9p-local.c
> +++ b/hw/9pfs/virtio-9p-local.c
> @@ -125,8 +125,7 @@ static int local_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
>      buffer = rpath(fs_ctx, path);
>      err =  lstat(buffer, stbuf);
>      if (err) {
> -        g_free(buffer);
> -        return err;
> +        goto err_out;
>      }
>      if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
>          /* Actual credentials are part of extended attrs */
> @@ -151,6 +150,7 @@ static int local_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
>          local_mapped_file_attr(fs_ctx, path, stbuf);
>      }
>  
> +err_out:
>      g_free(buffer);
>      return err;
>  }
> 

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed