Patchwork buildbot failure in qemu on default_x86_64_rhel5

login
register
mail settings
Submitter Aneesh Kumar K.V
Date Oct. 1, 2011, 9:33 a.m.
Message ID <87mxdl2h0c.fsf@skywalker.in.ibm.com>
Download mbox | patch
Permalink /patch/117266/
State New
Headers show

Comments

Aneesh Kumar K.V - Oct. 1, 2011, 9:33 a.m.
On Sat, 01 Oct 2011 14:28:47 +0530, "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> wrote:
> On Fri, 30 Sep 2011 09:26:53 +0100, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Fri, Sep 30, 2011 at 12:28 AM,  <qemu@buildbot.b1-systems.de> wrote:
> > > The Buildbot has detected a new failure on builder default_x86_64_rhel5 while building qemu.
> > > Full details are available at:
> > >  http://buildbot.b1-systems.de/qemu/builders/default_x86_64_rhel5/builds/24
> > 
> > Build error on RHEL 5 in virtio-9p-handle.c:
> > /home/buildbot/slave-public/default_x86_64_rhel5/build/hw/9pfs/virtio-9p-handle.c:
> > In function 'handle_utimensat':
> > /home/buildbot/slave-public/default_x86_64_rhel5/build/hw/9pfs/virtio-9p-handle.c:377:
> > warning: implicit declaration of function 'futimens'
> > /home/buildbot/slave-public/default_x86_64_rhel5/build/hw/9pfs/virtio-9p-handle.c:377:
> > warning: nested extern declaration of 'futimens'
> > 
> > RHEL 5 only has glibc 2.5 but futimens(2) was introduced in glibc 2.6.
> 
> We can make handle only available to glibc 2.6 and above . Handle fs
> drive will anyhow require a 2.6.39 kernel. Something like.
> 

I guess the below is better. I haven't built test the changes on rhel5
yet.

From c8e781fc8077587d23fcf658c14b5992282563a8 Mon Sep 17 00:00:00 2001
From: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Date: Sat, 1 Oct 2011 14:59:42 +0530
Subject: [PATCH] hw/9pfs: Fix build error on platform that don't support futimens

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-handle.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)
Stefan Hajnoczi - Oct. 3, 2011, 7:37 a.m.
On Sat, Oct 01, 2011 at 03:03:23PM +0530, Aneesh Kumar K.V wrote:
> +#ifndef CONFIG_UTIMENSAT
> +    /*
> +     * We support handle fs driver only if all related
> +     * syscalls are provided by host.
> +     */

Perhaps a ./configure check should be added to see whether the handle
syscalls are supported instead of using CONFIG_UTIMENSAT.

Stefan
Aneesh Kumar K.V - Oct. 3, 2011, 10:40 a.m.
On Mon, 3 Oct 2011 08:37:52 +0100, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Sat, Oct 01, 2011 at 03:03:23PM +0530, Aneesh Kumar K.V wrote:
> > +#ifndef CONFIG_UTIMENSAT
> > +    /*
> > +     * We support handle fs driver only if all related
> > +     * syscalls are provided by host.
> > +     */
> 
> Perhaps a ./configure check should be added to see whether the handle
> syscalls are supported instead of using CONFIG_UTIMENSAT.
> 

We already do check for handle syscall. Since glibc doesn't have the
this syscall yet, I added the check in virtio-9p-handle.c as below

#ifdef __NR_name_to_handle_at
static inline int name_to_handle(int dirfd, const char *name,
                                 struct file_handle *fh, int *mnt_id, int flags)
{
    return syscall(__NR_name_to_handle_at, dirfd, name, fh, mnt_id, flags);
}
#else
static inline int name_to_handle(int dirfd, const char *name,
                                 struct file_handle *fh, int *mnt_id, int flags)
{
    errno = ENOSYS;
    return -1;
}


-aneesh
Stefan Hajnoczi - Oct. 4, 2011, 7:18 a.m.
On Mon, Oct 03, 2011 at 04:10:50PM +0530, Aneesh Kumar K.V wrote:
> On Mon, 3 Oct 2011 08:37:52 +0100, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Sat, Oct 01, 2011 at 03:03:23PM +0530, Aneesh Kumar K.V wrote:
> > > +#ifndef CONFIG_UTIMENSAT
> > > +    /*
> > > +     * We support handle fs driver only if all related
> > > +     * syscalls are provided by host.
> > > +     */
> > 
> > Perhaps a ./configure check should be added to see whether the handle
> > syscalls are supported instead of using CONFIG_UTIMENSAT.
> > 
> 
> We already do check for handle syscall. Since glibc doesn't have the
> this syscall yet, I added the check in virtio-9p-handle.c as below

CONFIG_UTIMENSAT is defined when the host has glibc >= 2.6.

Handle syscalls are available on Linux 2.6.39 but not exposed by glibc.

Therefore CONFIG_UTIMENSAT has nothing to do with handle syscalls and
does not mean handle syscalls are supported.  I would drop that hunk of
the patch or test for the actual handle syscalls in ./configure.

Stefan
Aneesh Kumar K.V - Oct. 4, 2011, 8:48 a.m.
On Tue, 4 Oct 2011 08:18:07 +0100, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Oct 03, 2011 at 04:10:50PM +0530, Aneesh Kumar K.V wrote:
> > On Mon, 3 Oct 2011 08:37:52 +0100, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > > On Sat, Oct 01, 2011 at 03:03:23PM +0530, Aneesh Kumar K.V wrote:
> > > > +#ifndef CONFIG_UTIMENSAT
> > > > +    /*
> > > > +     * We support handle fs driver only if all related
> > > > +     * syscalls are provided by host.
> > > > +     */
> > > 
> > > Perhaps a ./configure check should be added to see whether the handle
> > > syscalls are supported instead of using CONFIG_UTIMENSAT.
> > > 
> > 
> > We already do check for handle syscall. Since glibc doesn't have the
> > this syscall yet, I added the check in virtio-9p-handle.c as below
> 
> CONFIG_UTIMENSAT is defined when the host has glibc >= 2.6.
> 
> Handle syscalls are available on Linux 2.6.39 but not exposed by glibc.
> 
> Therefore CONFIG_UTIMENSAT has nothing to do with handle syscalls and
> does not mean handle syscalls are supported.  I would drop that hunk of
> the patch or test for the actual handle syscalls in ./configure.

Here is what i am trying to achieve with the patch. For handle based fs
driver to work we need to have 

1) support for handle syscall
2) support for fd based syscalls like futimens, fstatat, readlinkat,
fchmod, fchownat, openat etc.

Now handle syscalls are recently added to kernel and glibc doesn't have
support for that. So what we did is to add handle syscall in
virtio-9p-handle.c via syscall(2). Now if the syscall is not supported
by the host kernel we will get ENOSYS. I only added support for i386 and
x86_64, because most the syscall number varies with different archs. For
other archs the wrapper returns ENOSYS. So instead of checking for
handle syscalls in configure script we did the above to make sure it
work without failure in most of the case. Once we have glibc support for
handle syscall the above changes should be dropped in favor of
configure script test.

Now for the fd based syscall dependency, we didn't initially had any
check for that because my expectation was most glibc should
have support for them. But RHEL 5 build failure indicate that futimens
is not supported. We were already checking for futimens in configure so
i added changes to make sure if futimens is not supported
handle_utimensat returns error. (That was not added as a run time
check, but rather a compile error fix). Now should we allow handle based fs
driver if futimens is not supported. I was suggesting we should not;
hence the check in init to return error when we don't support
futimens. The later part of init routine do check whether handle
syscalls are supported and disable handle fs driver if they are not.


-aneesh
Stefan Hajnoczi - Oct. 4, 2011, 10:29 a.m.
On Tue, Oct 04, 2011 at 02:18:20PM +0530, Aneesh Kumar K.V wrote:
> On Tue, 4 Oct 2011 08:18:07 +0100, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Mon, Oct 03, 2011 at 04:10:50PM +0530, Aneesh Kumar K.V wrote:
> > > On Mon, 3 Oct 2011 08:37:52 +0100, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > > > On Sat, Oct 01, 2011 at 03:03:23PM +0530, Aneesh Kumar K.V wrote:
> > > > > +#ifndef CONFIG_UTIMENSAT
> > > > > +    /*
> > > > > +     * We support handle fs driver only if all related
> > > > > +     * syscalls are provided by host.
> > > > > +     */
> > > > 
> > > > Perhaps a ./configure check should be added to see whether the handle
> > > > syscalls are supported instead of using CONFIG_UTIMENSAT.
> > > > 
> > > 
> > > We already do check for handle syscall. Since glibc doesn't have the
> > > this syscall yet, I added the check in virtio-9p-handle.c as below
> > 
> > CONFIG_UTIMENSAT is defined when the host has glibc >= 2.6.
> > 
> > Handle syscalls are available on Linux 2.6.39 but not exposed by glibc.
> > 
> > Therefore CONFIG_UTIMENSAT has nothing to do with handle syscalls and
> > does not mean handle syscalls are supported.  I would drop that hunk of
> > the patch or test for the actual handle syscalls in ./configure.
> 
> Here is what i am trying to achieve with the patch. For handle based fs
> driver to work we need to have 
> 
> 1) support for handle syscall
> 2) support for fd based syscalls like futimens, fstatat, readlinkat,
> fchmod, fchownat, openat etc.
> 
> Now handle syscalls are recently added to kernel and glibc doesn't have
> support for that. So what we did is to add handle syscall in
> virtio-9p-handle.c via syscall(2). Now if the syscall is not supported
> by the host kernel we will get ENOSYS. I only added support for i386 and
> x86_64, because most the syscall number varies with different archs. For
> other archs the wrapper returns ENOSYS. So instead of checking for
> handle syscalls in configure script we did the above to make sure it
> work without failure in most of the case. Once we have glibc support for
> handle syscall the above changes should be dropped in favor of
> configure script test.
> 
> Now for the fd based syscall dependency, we didn't initially had any
> check for that because my expectation was most glibc should
> have support for them. But RHEL 5 build failure indicate that futimens
> is not supported. We were already checking for futimens in configure so
> i added changes to make sure if futimens is not supported
> handle_utimensat returns error. (That was not added as a run time
> check, but rather a compile error fix). Now should we allow handle based fs
> driver if futimens is not supported. I was suggesting we should not;
> hence the check in init to return error when we don't support
> futimens. The later part of init routine do check whether handle
> syscalls are supported and disable handle fs driver if they are not.

Okay, then the comment should be:

/*
 * We support handle fs driver only if futimens is provided by the host
 */

The scenario where it might be possible to hit the CONFIG_UTIMENSAT is
with a modern kernel paired with an old userspace.  The handle syscall
which we call directly would succeed but the futimens(2) would not be
available.

On a sane system the handle syscall fails because the kernel doesn't
support it (and futimens isn't there either).

Stefan

Patch

diff --git a/hw/9pfs/virtio-9p-handle.c b/hw/9pfs/virtio-9p-handle.c
index 860b0e3..9e9ceb3 100644
--- a/hw/9pfs/virtio-9p-handle.c
+++ b/hw/9pfs/virtio-9p-handle.c
@@ -386,12 +386,17 @@  static int handle_utimensat(FsContext *ctx, V9fsPath *fs_path,
     int fd, ret;
     struct handle_data *data = (struct handle_data *)ctx->private;
 
+#ifdef CONFIG_UTIMENSAT
     fd = open_by_handle(data->mountfd, fs_path->data, O_NONBLOCK);
     if (fd < 0) {
         return fd;
     }
     ret = futimens(fd, buf);
     close(fd);
+#else
+    ret = -1;
+    errno = ENOSYS;
+#endif
     return ret;
 }
 
@@ -591,8 +596,16 @@  static int handle_init(FsContext *ctx)
     int ret, mnt_id;
     struct statfs stbuf;
     struct file_handle fh;
-    struct handle_data *data = g_malloc(sizeof(struct handle_data));
+    struct handle_data *data;
 
+#ifndef CONFIG_UTIMENSAT
+    /*
+     * We support handle fs driver only if all related
+     * syscalls are provided by host.
+     */
+    return -1;
+#endif
+    data = g_malloc(sizeof(struct handle_data));
     data->mountfd = open(ctx->fs_root, O_DIRECTORY);
     if (data->mountfd < 0) {
         ret = data->mountfd;