From patchwork Mon May 2 22:41:23 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: jvrao X-Patchwork-Id: 93721 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id DE1BC1007DE for ; Tue, 3 May 2011 08:41:46 +1000 (EST) Received: from localhost ([::1]:58678 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QH1oG-00040D-5p for incoming@patchwork.ozlabs.org; Mon, 02 May 2011 18:41:44 -0400 Received: from eggs.gnu.org ([140.186.70.92]:51524) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QH1o7-000405-HJ for qemu-devel@nongnu.org; Mon, 02 May 2011 18:41:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QH1o6-0001xF-0U for qemu-devel@nongnu.org; Mon, 02 May 2011 18:41:35 -0400 Received: from e33.co.us.ibm.com ([32.97.110.151]:53444) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QH1o5-0001wz-N8 for qemu-devel@nongnu.org; Mon, 02 May 2011 18:41:33 -0400 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e33.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p42MYSYL021347 for ; Mon, 2 May 2011 16:34:28 -0600 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p42MgUuG131720 for ; Mon, 2 May 2011 16:42:32 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p42MevR8008166 for ; Mon, 2 May 2011 16:40:57 -0600 Received: from oc6675851006.ibm.com (dyn9047029068.beaverton.ibm.com [9.47.29.68]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p42MeuDg008153 for ; Mon, 2 May 2011 16:40:56 -0600 Message-ID: <4DBF3313.7040203@linux.vnet.ibm.com> Date: Mon, 02 May 2011 15:41:23 -0700 From: Venkateswararao Jujjuri User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.15) Gecko/20110303 Thunderbird/3.1.9 MIME-Version: 1.0 To: qemu-devel@nongnu.org References: <1303998537-18128-1-git-send-email-sassan@sassan.me.uk> In-Reply-To: <1303998537-18128-1-git-send-email-sassan@sassan.me.uk> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6, seldom 2.4 (older, 4) X-Received-From: 32.97.110.151 Subject: Re: [Qemu-devel] [PATCH] Fix bug with virtio-9p xattr functions return values X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On 04/28/2011 06:48 AM, Sassan Panahinejad wrote: > Several functions in virtio-9p-xattr.c are passing the return value of the associated host system call back to the client instead of errno. > Since this value is -1 for any error (which, when received in the guest app as errno, indicates "operation not permitted") it is causing guest applications to fail in cases where the operation is not supported by the host. > This causes a number of problems with dpkg and with install. > This patch fixes the bug and returns the correct value, which means that guest applications are able to handle the error correctly. > > Signed-off-by: Sassan Panahinejad The motivation and direction of this change is correct but unfortunately this is little bigger than what is being identified here. This problem is not just limited to xattrs. Unfortunately this is all over the place. If you look at the post* functions of say, v9fs_do_lgetxattr( which is nothing but local_lgetxattr) it tries to capture errno. v9fs_post_xattr_getvalue() v9fs_post_lxattr_check() etc. But this is not always the case say, in v9fs_xattr_fid_clunk() retval = v9fs_do_lsetxattr(retval = v9fs_do_lsetxattr()). So the plan is to change this from top to bottom. Starting from the virtio-9p.c to the virtio-9p-local.c to other files and functions. Just as you did, we need to push the errno to the source of the system call and from then on, just use the function return values populated up. There should not be any errno references other than immediately after the systemcall(). Something like below. But I will be posting an initial patch which moves all the errno into v9fs_do*() and then into local.c . It will be great if you want to take this all the way. :) > --- > hw/virtio-9p-xattr.c | 21 ++++++++++++++++++--- > 1 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/hw/virtio-9p-xattr.c b/hw/virtio-9p-xattr.c > index 1aab081..fd6d892 100644 > --- a/hw/virtio-9p-xattr.c > +++ b/hw/virtio-9p-xattr.c > @@ -32,9 +32,14 @@ static XattrOperations *get_xattr_operations(XattrOperations **h, > ssize_t v9fs_get_xattr(FsContext *ctx, const char *path, > const char *name, void *value, size_t size) > { > + int ret; > XattrOperations *xops = get_xattr_operations(ctx->xops, name); > if (xops) { > - return xops->getxattr(ctx, path, name, value, size); > + ret = xops->getxattr(ctx, path, name, value, size); > + if (ret< 0) { > + return -errno; > + } > + return ret; > } > errno = -EOPNOTSUPP; > return -1; > @@ -117,9 +122,14 @@ err_out: > int v9fs_set_xattr(FsContext *ctx, const char *path, const char *name, > void *value, size_t size, int flags) > { > + int ret; > XattrOperations *xops = get_xattr_operations(ctx->xops, name); > if (xops) { > - return xops->setxattr(ctx, path, name, value, size, flags); > + ret = xops->setxattr(ctx, path, name, value, size, flags); > + if (ret< 0) { > + return -errno; > + } > + return ret; > } > errno = -EOPNOTSUPP; > return -1; > @@ -129,9 +139,14 @@ int v9fs_set_xattr(FsContext *ctx, const char *path, const char *name, > int v9fs_remove_xattr(FsContext *ctx, > const char *path, const char *name) > { > + int ret; > XattrOperations *xops = get_xattr_operations(ctx->xops, name); > if (xops) { > - return xops->removexattr(ctx, path, name); > + ret = xops->removexattr(ctx, path, name); > + if (ret< 0) { > + return -errno; > + } > + return ret; > } > errno = -EOPNOTSUPP; > return -1; diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c index 0a015de..1674f40 100644 --- a/hw/9pfs/virtio-9p-local.c +++ b/hw/9pfs/virtio-9p-local.c @@ -143,9 +143,10 @@ static int local_open(FsContext *ctx, const char *path, int return open(rpath(ctx, path), flags); } -static DIR *local_opendir(FsContext *ctx, const char *path) +static int local_opendir(FsContext *ctx, const char *path, DIR **dir) { - return opendir(rpath(ctx, path)); + *dir = opendir(rpath(ctx, path)); + return *dir ? 0 : -errno; } static void local_rewinddir(FsContext *ctx, DIR *dir) diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c index b5fc52b..7d88f48 100644 --- a/hw/9pfs/virtio-9p.c +++ b/hw/9pfs/virtio-9p.c @@ -110,9 +110,9 @@ static int v9fs_do_open(V9fsState *s, V9fsString *path, int return s->ops->open(&s->ctx, path->data, flags); } -static DIR *v9fs_do_opendir(V9fsState *s, V9fsString *path) +static int v9fs_do_opendir(V9fsState *s, V9fsString *path, DIR **dir) { - return s->ops->opendir(&s->ctx, path->data); + return s->ops->opendir(&s->ctx, path->data, dir); } static void v9fs_do_rewinddir(V9fsState *s, DIR *dir) @@ -1665,8 +1665,7 @@ static int32_t get_iounit(V9fsState *s, V9fsString *name) static void v9fs_open_post_opendir(V9fsState *s, V9fsOpenState *vs, int err) { - if (vs->fidp->fs.dir == NULL) { - err = -errno; + if (err < 0) { goto out; } vs->fidp->fid_type = P9_FID_DIR; @@ -1714,7 +1713,7 @@ static void v9fs_open_post_lstat(V9fsState *s, V9fsOpenSta stat_to_qid(&vs->stbuf, &vs->qid); if (S_ISDIR(vs->stbuf.st_mode)) { - vs->fidp->fs.dir = v9fs_do_opendir(s, &vs->fidp->path); + err = v9fs_do_opendir(s, &vs->fidp->path, &vs->fidp->fs.dir); v9fs_open_post_opendir(s, vs, err); } else { if (s->proto_version == V9FS_PROTO_2000L) { @@ -2397,9 +2396,6 @@ static void v9fs_create_post_perms(V9fsState *s, V9fsCreat static void v9fs_create_post_opendir(V9fsState *s, V9fsCreateState *vs, int err) { - if (!vs->fidp->fs.dir) { - err = -errno; - } vs->fidp->fid_type = P9_FID_DIR; v9fs_post_create(s, vs, err); } @@ -2412,7 +2408,7 @@ static void v9fs_create_post_dir_lstat(V9fsState *s, V9fsC goto out; } - vs->fidp->fs.dir = v9fs_do_opendir(s, &vs->fullname); + err = v9fs_do_opendir(s, &vs->fullname, &vs->fidp->fs.dir); v9fs_create_post_opendir(s, vs, err); return; diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h index 126e60e..dc95824 100644 --- a/hw/file-op-9p.h +++ b/hw/file-op-9p.h @@ -73,7 +73,7 @@ typedef struct FileOperations int (*setuid)(FsContext *, uid_t); int (*close)(FsContext *, int); int (*closedir)(FsContext *, DIR *); - DIR *(*opendir)(FsContext *, const char *); + int (*opendir)(FsContext *, const char *, DIR **); int (*open)(FsContext *, const char *, int); int (*open2)(FsContext *, const char *, int, FsCred *); void (*rewinddir)(FsContext *, DIR *);