[Qemu-devel] qemu-kvm build issue on RHEL5.1

[Qemu-devel] qemu-kvm build issue on RHEL5.1

[Hao, Xudong]

Hi, 
Currently qemu-kvm build fail on RHEL5 with gcc 4.1.2, build can pass on Fedora11 with gcc 4.4.1, can anybody look on RHEL5 system?

Gcc: 4.1.2
system: RHEL5.1
qemu-kvm: 85566812a4f8cae721fea0224e05a7e75c08c5dd

...
  LINK  qemu-img
  LINK  qemu-io
  CC    libhw64/virtio-9p-local.o
cc1: warnings being treated as errors
/home/source/qemu-kvm/hw/virtio-9p-local.c: In function 'local_utimensat':
/home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
/home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
make[1]: *** [virtio-9p-local.o] Error 1
make: *** [subdir-libhw64] Error 2


Best Regards,
Xudong Hao

Re: [Qemu-devel] qemu-kvm build issue on RHEL5.1

[Hidetoshi Seto]

(Add CC to kvm@vger)

(2010/10/12 10:52), Hao, Xudong wrote:
> Hi, 
> Currently qemu-kvm build fail on RHEL5 with gcc 4.1.2, build can pass on Fedora11 with gcc 4.4.1, can anybody look on RHEL5 system?
> 
> Gcc: 4.1.2
> system: RHEL5.1
> qemu-kvm: 85566812a4f8cae721fea0224e05a7e75c08c5dd
> 
> ...
>   LINK  qemu-img
>   LINK  qemu-io
>   CC    libhw64/virtio-9p-local.o
> cc1: warnings being treated as errors
> /home/source/qemu-kvm/hw/virtio-9p-local.c: In function 'local_utimensat':
> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
> make[1]: *** [virtio-9p-local.o] Error 1
> make: *** [subdir-libhw64] Error 2
> 
> 
> Best Regards,
> Xudong Hao

It seems that this issue is caused by the old glibc.
Though I don't know well about virtio-9p and suppose there
should be better fix, I confirmed that following change
removed the warnings.

Thanks,
H.Seto

=====

[PATCH] virtio-9p: fix build on !CONFIG_UTIMENSAT

This removes following warnings on RHEL5, which has utimensat syscall
but has old glibc that doesn't have support for it:

hw/virtio-9p-local.c: In function 'local_utimensat':
hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'

and

hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this function)
hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once
hw/virtio-9p.c:1410: error: for each function it appears in.)
hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this function)
hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this function)

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 hw/virtio-9p-local.c |    8 ++++++++
 hw/virtio-9p.c       |    9 +++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
index 57f9243..e075c27 100644
--- a/hw/virtio-9p-local.c
+++ b/hw/virtio-9p-local.c
@@ -18,6 +18,9 @@
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <attr/xattr.h>
+#ifndef CONFIG_UTIMENSAT
+#include <syscall.h>
+#endif
 
 static const char *rpath(FsContext *ctx, const char *path)
 {
@@ -476,7 +479,12 @@ static int local_chown(FsContext *fs_ctx, const char *path, FsCred *credp)
 static int local_utimensat(FsContext *s, const char *path,
 		       const struct timespec *buf)
 {
+#ifndef CONFIG_UTIMENSAT
+    return syscall(SYS_utimensat, AT_FDCWD, rpath(s, path), buf,
+                   AT_SYMLINK_NOFOLLOW);
+#else
     return utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+#endif
 }
 
 static int local_remove(FsContext *ctx, const char *path)
diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 32fa3bc..efe5c51 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1393,6 +1393,15 @@ out:
     qemu_free(vs);
 }
 
+#ifndef CONFIG_UTIMENSAT
+#ifndef UTIME_NOW
+# define UTIME_NOW	((1l << 30) - 1l)
+#endif
+#ifndef UTIME_OMIT
+# define UTIME_OMIT	((1l << 30) - 2l)
+#endif
+#endif
+
 static void v9fs_setattr_post_chmod(V9fsState *s, V9fsSetattrState *vs, int err)
 {
     if (err == -1) {
-- 
1.7.3.1

RE: [Qemu-devel] qemu-kvm build issue on RHEL5.1

[Hao, Xudong]

Hidetoshi Seto wrote:
> (Add CC to kvm@vger)
> 
> (2010/10/12 10:52), Hao, Xudong wrote:
>> Hi,
>> Currently qemu-kvm build fail on RHEL5 with gcc 4.1.2, build can
>> pass on Fedora11 with gcc 4.4.1, can anybody look on RHEL5 system? 
>> 
>> Gcc: 4.1.2
>> system: RHEL5.1
>> qemu-kvm: 85566812a4f8cae721fea0224e05a7e75c08c5dd
>> 
>> ...
>>   LINK  qemu-img
>>   LINK  qemu-io
>>   CC    libhw64/virtio-9p-local.o
>> cc1: warnings being treated as errors
>> /home/source/qemu-kvm/hw/virtio-9p-local.c: In function
>> 'local_utimensat': /home/source/qemu-kvm/hw/virtio-9p-local.c:479:
>> warning: implicit declaration of function 'utimensat'
>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: nested
>> extern declaration of 'utimensat'  
>> make[1]: *** [virtio-9p-local.o] Error 1
>> make: *** [subdir-libhw64] Error 2
>> 
>> 
>> Best Regards,
>> Xudong Hao
> 
> It seems that this issue is caused by the old glibc.
> Though I don't know well about virtio-9p and suppose there
> should be better fix, I confirmed that following change
> removed the warnings.
> 
> Thanks,
> H.Seto
> 
> =====
> 
> [PATCH] virtio-9p: fix build on !CONFIG_UTIMENSAT
> 
> This removes following warnings on RHEL5, which has utimensat syscall
> but has old glibc that doesn't have support for it:
> 
> hw/virtio-9p-local.c: In function 'local_utimensat':
> hw/virtio-9p-local.c:479: warning: implicit declaration of function
> 'utimensat' hw/virtio-9p-local.c:479: warning: nested extern
> declaration of 'utimensat' 
> 
> and
> 
> hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
> hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this
> function) hw/virtio-9p.c:1410: error: (Each undeclared identifier is
> reported only once hw/virtio-9p.c:1410: error: for each function it
> appears in.) 
> hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in
> this function) hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
> hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in
> this function) 
> 
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> ---
>  hw/virtio-9p-local.c |    8 ++++++++
>  hw/virtio-9p.c       |    9 +++++++++
>  2 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
> index 57f9243..e075c27 100644
> --- a/hw/virtio-9p-local.c
> +++ b/hw/virtio-9p-local.c
> @@ -18,6 +18,9 @@
>  #include <sys/socket.h>
>  #include <sys/un.h>
>  #include <attr/xattr.h>
> +#ifndef CONFIG_UTIMENSAT
> +#include <syscall.h>
> +#endif
> 
>  static const char *rpath(FsContext *ctx, const char *path)
>  {
> @@ -476,7 +479,12 @@ static int local_chown(FsContext *fs_ctx, const
>  char *path, FsCred *credp) static int local_utimensat(FsContext *s,
>  		       const char *path, const struct timespec *buf)
>  {
> +#ifndef CONFIG_UTIMENSAT
> +    return syscall(SYS_utimensat, AT_FDCWD, rpath(s, path), buf,
> +                   AT_SYMLINK_NOFOLLOW);
> +#else
>      return utimensat(AT_FDCWD, rpath(s, path), buf,
> AT_SYMLINK_NOFOLLOW); +#endif
>  }
> 
>  static int local_remove(FsContext *ctx, const char *path)
> diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
> index 32fa3bc..efe5c51 100644
> --- a/hw/virtio-9p.c
> +++ b/hw/virtio-9p.c
> @@ -1393,6 +1393,15 @@ out:
>      qemu_free(vs);
>  }
> 
> +#ifndef CONFIG_UTIMENSAT
> +#ifndef UTIME_NOW
> +# define UTIME_NOW	((1l << 30) - 1l)
> +#endif
> +#ifndef UTIME_OMIT
> +# define UTIME_OMIT	((1l << 30) - 2l)
> +#endif
> +#endif
> +
>  static void v9fs_setattr_post_chmod(V9fsState *s, V9fsSetattrState
>  *vs, int err) {
>      if (err == -1) {

Seto, your patch works fine for me, verified on my RHEL5 system.

Thanks,
Xudong

Re: [Qemu-devel] qemu-kvm build issue on RHEL5.1

[Blue Swirl]

On Wed, Oct 13, 2010 at 8:00 AM, Hidetoshi Seto
<seto.hidetoshi@jp.fujitsu.com> wrote:
> (Add CC to kvm@vger)
>
> (2010/10/12 10:52), Hao, Xudong wrote:
>> Hi,
>> Currently qemu-kvm build fail on RHEL5 with gcc 4.1.2, build can pass on Fedora11 with gcc 4.4.1, can anybody look on RHEL5 system?
>>
>> Gcc: 4.1.2
>> system: RHEL5.1
>> qemu-kvm: 85566812a4f8cae721fea0224e05a7e75c08c5dd
>>
>> ...
>>   LINK  qemu-img
>>   LINK  qemu-io
>>   CC    libhw64/virtio-9p-local.o
>> cc1: warnings being treated as errors
>> /home/source/qemu-kvm/hw/virtio-9p-local.c: In function 'local_utimensat':
>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
>> make[1]: *** [virtio-9p-local.o] Error 1
>> make: *** [subdir-libhw64] Error 2
>>
>>
>> Best Regards,
>> Xudong Hao
>
> It seems that this issue is caused by the old glibc.
> Though I don't know well about virtio-9p and suppose there
> should be better fix, I confirmed that following change
> removed the warnings.

But then the system call will be made blindly without checking if the
kernel supports utimensat(). At the minimum, there should be a sane
response to ENOSYS error.

What happens if the system headers do not define SYS_utimensat?

Re: [Qemu-devel] qemu-kvm build issue on RHEL5.1

[Hidetoshi Seto]

(2010/10/14 4:11), Blue Swirl wrote:
> On Wed, Oct 13, 2010 at 8:00 AM, Hidetoshi Seto
> <seto.hidetoshi@jp.fujitsu.com> wrote:
>> (Add CC to kvm@vger)
>>
>> (2010/10/12 10:52), Hao, Xudong wrote:
>>> Hi,
>>> Currently qemu-kvm build fail on RHEL5 with gcc 4.1.2, build can pass on Fedora11 with gcc 4.4.1, can anybody look on RHEL5 system?
>>>
>>> Gcc: 4.1.2
>>> system: RHEL5.1
>>> qemu-kvm: 85566812a4f8cae721fea0224e05a7e75c08c5dd
>>>
>>> ...
>>>   LINK  qemu-img
>>>   LINK  qemu-io
>>>   CC    libhw64/virtio-9p-local.o
>>> cc1: warnings being treated as errors
>>> /home/source/qemu-kvm/hw/virtio-9p-local.c: In function 'local_utimensat':
>>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
>>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
>>> make[1]: *** [virtio-9p-local.o] Error 1
>>> make: *** [subdir-libhw64] Error 2
>>>
>>>
>>> Best Regards,
>>> Xudong Hao
>>
>> It seems that this issue is caused by the old glibc.
>> Though I don't know well about virtio-9p and suppose there
>> should be better fix, I confirmed that following change
>> removed the warnings.
> 
> But then the system call will be made blindly without checking if the
> kernel supports utimensat(). At the minimum, there should be a sane
> response to ENOSYS error.

Yes. But I'm not sure how this virtio-9p should behave if there is
no utimensat.  I think it will be better to fix this warning first
to allow fellows using RHEL5 to restart contribute on qemu-kvm,
and change this issue to virtio-9p specific problem to allow
specialists of virtio-9p to have discussion for fix without
bothering other developers. 

... Or is it better to put abort() here instead of syscall?

> 
> What happens if the system headers do not define SYS_utimensat?

I suppose build will fail, say, we might need incremental patch
named "fix build on RHEL4" or so.
But I have no idea, whether there could be a workaround if there
is no utimensat, whether we could provide something like wrapper
named compat_utimensat or qemu_utimensat, and/or whether it makes
sense if virtio-9p have dependency with presence of utimensat.


Thanks,
H.Seto

Re: [Qemu-devel] qemu-kvm build issue on RHEL5.1

[Chris Wright]

* Hidetoshi Seto (seto.hidetoshi@jp.fujitsu.com) wrote:
> (2010/10/14 4:11), Blue Swirl wrote:
> > On Wed, Oct 13, 2010 at 8:00 AM, Hidetoshi Seto
> > <seto.hidetoshi@jp.fujitsu.com> wrote:
> >> (Add CC to kvm@vger)
> >>
> >> (2010/10/12 10:52), Hao, Xudong wrote:
> >>> Hi,
> >>> Currently qemu-kvm build fail on RHEL5 with gcc 4.1.2, build can pass on Fedora11 with gcc 4.4.1, can anybody look on RHEL5 system?
> >>>
> >>> Gcc: 4.1.2
> >>> system: RHEL5.1
> >>> qemu-kvm: 85566812a4f8cae721fea0224e05a7e75c08c5dd
> >>>
> >>> ...
> >>>   LINK  qemu-img
> >>>   LINK  qemu-io
> >>>   CC    libhw64/virtio-9p-local.o
> >>> cc1: warnings being treated as errors
> >>> /home/source/qemu-kvm/hw/virtio-9p-local.c: In function 'local_utimensat':
> >>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
> >>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
> >>> make[1]: *** [virtio-9p-local.o] Error 1
> >>> make: *** [subdir-libhw64] Error 2
> >>>
> >>>
> >>> Best Regards,
> >>> Xudong Hao
> >>
> >> It seems that this issue is caused by the old glibc.
> >> Though I don't know well about virtio-9p and suppose there
> >> should be better fix, I confirmed that following change
> >> removed the warnings.
> > 
> > But then the system call will be made blindly without checking if the
> > kernel supports utimensat(). At the minimum, there should be a sane
> > response to ENOSYS error.
> 
> Yes. But I'm not sure how this virtio-9p should behave if there is
> no utimensat.  I think it will be better to fix this warning first
> to allow fellows using RHEL5 to restart contribute on qemu-kvm,
> and change this issue to virtio-9p specific problem to allow
> specialists of virtio-9p to have discussion for fix without
> bothering other developers. 

One way to workaround this is to simply not install libattr-devel
(effecitvely disabling virtio-9p).

But I agree with Blue Swirl, need a better fallback plan.  A qemu local
implementation of something like qemu_utimensat() that simply uses
glibc/kernel interface when available and falls back to using utimes()
makes sense to me.  Then the worst case is loss of resolution from ns to
us.

thanks,
-chris

Re: [Qemu-devel] qemu-kvm build issue on RHEL5.1

[Hidetoshi Seto]

(2010/11/05 2:03), Chris Wright wrote:
> * Hidetoshi Seto (seto.hidetoshi@jp.fujitsu.com) wrote:
>> (2010/10/14 4:11), Blue Swirl wrote:
>>> On Wed, Oct 13, 2010 at 8:00 AM, Hidetoshi Seto
>>> <seto.hidetoshi@jp.fujitsu.com> wrote:
>>>> (Add CC to kvm@vger)
>>>>
>>>> (2010/10/12 10:52), Hao, Xudong wrote:
>>>>> Hi,
>>>>> Currently qemu-kvm build fail on RHEL5 with gcc 4.1.2, build can pass on Fedora11 with gcc 4.4.1, can anybody look on RHEL5 system?
>>>>>
>>>>> Gcc: 4.1.2
>>>>> system: RHEL5.1
>>>>> qemu-kvm: 85566812a4f8cae721fea0224e05a7e75c08c5dd
>>>>>
>>>>> ...
>>>>>   LINK  qemu-img
>>>>>   LINK  qemu-io
>>>>>   CC    libhw64/virtio-9p-local.o
>>>>> cc1: warnings being treated as errors
>>>>> /home/source/qemu-kvm/hw/virtio-9p-local.c: In function 'local_utimensat':
>>>>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
>>>>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
>>>>> make[1]: *** [virtio-9p-local.o] Error 1
>>>>> make: *** [subdir-libhw64] Error 2
>>>>>
>>>>>
>>>>> Best Regards,
>>>>> Xudong Hao
>>>>
>>>> It seems that this issue is caused by the old glibc.
>>>> Though I don't know well about virtio-9p and suppose there
>>>> should be better fix, I confirmed that following change
>>>> removed the warnings.
>>>
>>> But then the system call will be made blindly without checking if the
>>> kernel supports utimensat(). At the minimum, there should be a sane
>>> response to ENOSYS error.
>>
>> Yes. But I'm not sure how this virtio-9p should behave if there is
>> no utimensat.  I think it will be better to fix this warning first
>> to allow fellows using RHEL5 to restart contribute on qemu-kvm,
>> and change this issue to virtio-9p specific problem to allow
>> specialists of virtio-9p to have discussion for fix without
>> bothering other developers. 
> 
> One way to workaround this is to simply not install libattr-devel
> (effecitvely disabling virtio-9p).
> 
> But I agree with Blue Swirl, need a better fallback plan.  A qemu local
> implementation of something like qemu_utimensat() that simply uses
> glibc/kernel interface when available and falls back to using utimes()
> makes sense to me.  Then the worst case is loss of resolution from ns to
> us.

According to the commit 74bc02b2d2272dc88fb98d43e631eb154717f517, the
title "Do not reset atime" can tell us that the original motivation to
use utimensat() is not for the resolution.

Anyway, I agree to have something like qemu_utimensat().
I made a patch for the first step, and will post it next to this reply. 


Thanks,
H.Seto

[Qemu-devel] [PATCH] virtio-9p: fix build on !CONFIG_UTIMENSAT v2

[Hidetoshi Seto]

This patch introduce a fallback mechanism for old systems that do not
support utimensat.  This will fix build failure with following warnings:

hw/virtio-9p-local.c: In function 'local_utimensat':
hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'

and

hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this function)
hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once
hw/virtio-9p.c:1410: error: for each function it appears in.)
hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this function)
hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this function)

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 hw/virtio-9p-local.c |   32 ++++++++++++++++++++++++++++++--
 hw/virtio-9p.h       |   10 ++++++++++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
index 0d52020..7811d2c 100644
--- a/hw/virtio-9p-local.c
+++ b/hw/virtio-9p-local.c
@@ -479,10 +479,38 @@ static int local_chown(FsContext *fs_ctx, const char *path, FsCred *credp)
     return -1;
 }
 
+/* TODO: relocate this to proper file, and make it more generic */
+static int qemu_utimensat(int dirfd, const char *path,
+                          const struct timespec *times, int flags)
+{
+#ifdef CONFIG_UTIMENSAT
+    return utimensat(dirfd, path, times, flags);
+#else
+    /*
+     * Fallback: use utimes() instead of utimensat().
+     * See commit 74bc02b2d2272dc88fb98d43e631eb154717f517 for known problem.
+     */
+    struct timeval tv[2];
+    int i;
+
+    for (i = 0; i < 2; i++) {
+        if (times[i].tv_nsec == UTIME_OMIT || times[i].tv_nsec == UTIME_NOW) {
+            tv[i].tv_sec = 0;
+            tv[i].tv_usec = 0;
+        } else {
+            tv[i].tv_sec = times[i].tv_sec;
+            tv[i].tv_usec = times[i].tv_nsec / 1000;
+        }
+    }
+
+    return utimes(path, &tv[0]);
+#endif
+}
+
 static int local_utimensat(FsContext *s, const char *path,
-		       const struct timespec *buf)
+                           const struct timespec *buf)
 {
-    return utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+    return qemu_utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
 }
 
 static int local_remove(FsContext *ctx, const char *path)
diff --git a/hw/virtio-9p.h b/hw/virtio-9p.h
index 6c23319..d448d8a 100644
--- a/hw/virtio-9p.h
+++ b/hw/virtio-9p.h
@@ -8,6 +8,16 @@
 
 #include "file-op-9p.h"
 
+/* TODO: relocate this to proper file */
+#ifndef CONFIG_UTIMENSAT
+#ifndef UTIME_NOW
+# define UTIME_NOW     ((1l << 30) - 1l)
+#endif
+#ifndef UTIME_OMIT
+# define UTIME_OMIT    ((1l << 30) - 2l)
+#endif
+#endif
+
 /* The feature bitmap for virtio 9P */
 /* The mount point is specified in a config variable */
 #define VIRTIO_9P_MOUNT_TAG 0
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH] virtio-9p: fix build on !CONFIG_UTIMENSAT v2

[M. Mohan Kumar]

> This patch introduce a fallback mechanism for old systems that do not
> support utimensat.  This will fix build failure with following warnings:
> 
> hw/virtio-9p-local.c: In function 'local_utimensat':
> hw/virtio-9p-local.c:479: warning: implicit declaration of function
> 'utimensat' hw/virtio-9p-local.c:479: warning: nested extern declaration
> of 'utimensat'
> 
> and
> 
> hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
> hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this
> function) hw/virtio-9p.c:1410: error: (Each undeclared identifier is
> reported only once hw/virtio-9p.c:1410: error: for each function it
> appears in.)
> hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this
> function) hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
> hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this
> function)
> 
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> ---
>  hw/virtio-9p-local.c |   32 ++++++++++++++++++++++++++++++--
>  hw/virtio-9p.h       |   10 ++++++++++
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
> index 0d52020..7811d2c 100644
> --- a/hw/virtio-9p-local.c
> +++ b/hw/virtio-9p-local.c
> @@ -479,10 +479,38 @@ static int local_chown(FsContext *fs_ctx, const char
> *path, FsCred *credp) return -1;
>  }
> 
> +/* TODO: relocate this to proper file, and make it more generic */
> +static int qemu_utimensat(int dirfd, const char *path,
> +                          const struct timespec *times, int flags)
> +{

IMHO, this code can be moved to cutils.c

> +#ifdef CONFIG_UTIMENSAT
> +    return utimensat(dirfd, path, times, flags);
> +#else
> +    /*
> +     * Fallback: use utimes() instead of utimensat().
> +     * See commit 74bc02b2d2272dc88fb98d43e631eb154717f517 for known
> problem. +     */
> +    struct timeval tv[2];
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        if (times[i].tv_nsec == UTIME_OMIT || times[i].tv_nsec ==
> UTIME_NOW) { +            tv[i].tv_sec = 0;
> +            tv[i].tv_usec = 0;
> +        } else {
> +            tv[i].tv_sec = times[i].tv_sec;
> +            tv[i].tv_usec = times[i].tv_nsec / 1000;
> +        }
> +    }
> +
> +    return utimes(path, &tv[0]);
> +#endif

The idea of introducing utimensat was to avoid resetting atime to 1970-01-01 
05:30:00 (utime does not give option to not change atime). But as per utimes 
man page, if any of the time field is 0, it would be set to current time. As 
per stat man page, truncate will not update atime, only mtime will be updated.

Re: [Qemu-devel] [PATCH] virtio-9p: fix build on !CONFIG_UTIMENSAT v2

[Jes Sorensen]

On 11/08/10 07:44, M. Mohan Kumar wrote:
>> This patch introduce a fallback mechanism for old systems that do not
>> support utimensat.  This will fix build failure with following warnings:
>>
>> hw/virtio-9p-local.c: In function 'local_utimensat':
>> hw/virtio-9p-local.c:479: warning: implicit declaration of function
>> 'utimensat' hw/virtio-9p-local.c:479: warning: nested extern declaration
>> of 'utimensat'
>>
>> and
>>
>> hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
>> hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this
>> function) hw/virtio-9p.c:1410: error: (Each undeclared identifier is
>> reported only once hw/virtio-9p.c:1410: error: for each function it
>> appears in.)
>> hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this
>> function) hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
>> hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this
>> function)
>>
>> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
>> ---
>>  hw/virtio-9p-local.c |   32 ++++++++++++++++++++++++++++++--
>>  hw/virtio-9p.h       |   10 ++++++++++
>>  2 files changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
>> index 0d52020..7811d2c 100644
>> --- a/hw/virtio-9p-local.c
>> +++ b/hw/virtio-9p-local.c
>> @@ -479,10 +479,38 @@ static int local_chown(FsContext *fs_ctx, const char
>> *path, FsCred *credp) return -1;
>>  }
>>
>> +/* TODO: relocate this to proper file, and make it more generic */
>> +static int qemu_utimensat(int dirfd, const char *path,
>> +                          const struct timespec *times, int flags)
>> +{
> 
> IMHO, this code can be moved to cutils.c

It's not a C utility function, so it really belongs in oslib-posix.c,
but otherwise I agree. This is emulation of a C library function, it
shouldn't be in the 9p code.

Cheers,
Jes

[Qemu-devel] Re: [PATCH] virtio-9p: fix build on !CONFIG_UTIMENSAT v2

[Chris Wright]

* Hidetoshi Seto (seto.hidetoshi@jp.fujitsu.com) wrote:
> +    /*
> +     * Fallback: use utimes() instead of utimensat().
> +     * See commit 74bc02b2d2272dc88fb98d43e631eb154717f517 for known problem.
> +     */
> +    struct timeval tv[2];
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        if (times[i].tv_nsec == UTIME_OMIT || times[i].tv_nsec == UTIME_NOW) {
> +            tv[i].tv_sec = 0;
> +            tv[i].tv_usec = 0;

I don't think this is accurate in either case.  It will set the
atime, mtime, or both to 0.

For UTIME_NOW (in both) we'd simply pass NULL to utimes(2).  For
UTIME_OMIT (in both) we'd simply skip the call to utimes(2) altogether.

The harder part is a mixed mode (i.e. the truncate fix mentioned in the
above commit).  I think the only way to handle UTIME_NOW in one is to
call gettimeofday (or clock_gettime for better resolution) to find out
what current time is.  And for UTIME_OMIT call stat to find out what the
current setting is and reset to that value.  Both of those cases can
possibly zero out the extra precision (providing only seconds
resolution).

thanks,
-chris

Re: [Qemu-devel] Re: [PATCH] virtio-9p: fix build on !CONFIG_UTIMENSAT v2

[Hidetoshi Seto]

(2010/11/14 14:58), Chris Wright wrote:
> * Hidetoshi Seto (seto.hidetoshi@jp.fujitsu.com) wrote:
>> +    /*
>> +     * Fallback: use utimes() instead of utimensat().
>> +     * See commit 74bc02b2d2272dc88fb98d43e631eb154717f517 for known problem.
>> +     */
>> +    struct timeval tv[2];
>> +    int i;
>> +
>> +    for (i = 0; i < 2; i++) {
>> +        if (times[i].tv_nsec == UTIME_OMIT || times[i].tv_nsec == UTIME_NOW) {
>> +            tv[i].tv_sec = 0;
>> +            tv[i].tv_usec = 0;
> 
> I don't think this is accurate in either case.  It will set the
> atime, mtime, or both to 0.
> 
> For UTIME_NOW (in both) we'd simply pass NULL to utimes(2).  For
> UTIME_OMIT (in both) we'd simply skip the call to utimes(2) altogether.
> 
> The harder part is a mixed mode (i.e. the truncate fix mentioned in the
> above commit).  I think the only way to handle UTIME_NOW in one is to
> call gettimeofday (or clock_gettime for better resolution) to find out
> what current time is.  And for UTIME_OMIT call stat to find out what the
> current setting is and reset to that value.  Both of those cases can
> possibly zero out the extra precision (providing only seconds
> resolution).

Thank you for comments!
I'll post an updated one soon.

Thanks,
H.Seto

[Qemu-devel] [PATCH v3] virtio-9p: fix build on !CONFIG_UTIMENSAT

[Hidetoshi Seto]

This patch introduce a fallback mechanism for old systems that do not
support utimensat().  This fix build failure with following warnings:

hw/virtio-9p-local.c: In function 'local_utimensat':
hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'

and:

hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this function)
hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once
hw/virtio-9p.c:1410: error: for each function it appears in.)
hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this function)
hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this function)

v3:
  - Use better alternative handling for UTIME_NOW/OMIT
  - Move qemu_utimensat() to cutils.c
V2:
  - Introduce qemu_utimensat()

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 cutils.c             |   43 +++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-9p-local.c |    4 ++--
 qemu-common.h        |   10 ++++++++++
 3 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/cutils.c b/cutils.c
index 536ee93..3c18941 100644
--- a/cutils.c
+++ b/cutils.c
@@ -288,3 +288,46 @@ int fcntl_setfl(int fd, int flag)
 }
 #endif
 
+int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
+                   int flags)
+{
+#ifdef CONFIG_UTIMENSAT
+    return utimensat(dirfd, path, times, flags);
+#else
+    /* Fallback: use utimes() instead of utimensat() */
+    struct timeval tv[2], tv_now;
+    struct stat st;
+    int i;
+
+    /* happy if special cases */
+    if (times[0].tv_nsec == UTIME_OMIT && times[1].tv_nsec == UTIME_OMIT) {
+        return 0;
+    }
+    if (times[0].tv_nsec == UTIME_NOW && times[1].tv_nsec == UTIME_NOW) {
+        return utimes(path, NULL);
+    }
+
+    /* prepare for hard cases */
+    if (times[0].tv_nsec == UTIME_NOW || times[1].tv_nsec == UTIME_NOW) {
+        gettimeofday(&tv_now, NULL);
+    }
+    if (times[0].tv_nsec == UTIME_OMIT || times[1].tv_nsec == UTIME_OMIT) {
+        stat(path, &st);
+    }
+
+    for (i = 0; i < 2; i++) {
+        if (times[i].tv_nsec == UTIME_NOW) {
+            tv[i].tv_sec = tv_now.tv_sec;
+            tv[i].tv_usec = 0;
+        } else if (times[i].tv_nsec == UTIME_OMIT) {
+            tv[i].tv_sec = (i == 0) ? st.st_atime : st.st_mtime;
+            tv[i].tv_usec = 0;
+        } else {
+            tv[i].tv_sec = times[i].tv_sec;
+            tv[i].tv_usec = times[i].tv_nsec / 1000;
+        }
+    }
+
+    return utimes(path, &tv[0]);
+#endif
+}
diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
index 0d52020..41603ea 100644
--- a/hw/virtio-9p-local.c
+++ b/hw/virtio-9p-local.c
@@ -480,9 +480,9 @@ static int local_chown(FsContext *fs_ctx, const char *path, FsCred *credp)
 }
 
 static int local_utimensat(FsContext *s, const char *path,
-		       const struct timespec *buf)
+                           const struct timespec *buf)
 {
-    return utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+    return qemu_utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
 }
 
 static int local_remove(FsContext *ctx, const char *path)
diff --git a/qemu-common.h b/qemu-common.h
index 2fbc27f..7fe4c16 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -146,6 +146,16 @@ time_t mktimegm(struct tm *tm);
 int qemu_fls(int i);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
+#ifndef CONFIG_UTIMENSAT
+#ifndef UTIME_NOW
+# define UTIME_NOW     ((1l << 30) - 1l)
+#endif
+#ifndef UTIME_OMIT
+# define UTIME_OMIT    ((1l << 30) - 2l)
+#endif
+#endif
+int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
+    int flags);
 
 /* path.c */
 void init_paths(const char *prefix);
-- 
1.7.3.1

[Qemu-devel] Re: [PATCH v3] virtio-9p: fix build on !CONFIG_UTIMENSAT

[Chris Wright]

* Hidetoshi Seto (seto.hidetoshi@jp.fujitsu.com) wrote:
> This patch introduce a fallback mechanism for old systems that do not
> support utimensat().  This fix build failure with following warnings:
> 
> hw/virtio-9p-local.c: In function 'local_utimensat':
> hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
> hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
> 
> and:
> 
> hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
> hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this function)
> hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once
> hw/virtio-9p.c:1410: error: for each function it appears in.)
> hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this function)
> hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
> hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this function)
> 
> v3:
>   - Use better alternative handling for UTIME_NOW/OMIT
>   - Move qemu_utimensat() to cutils.c
> V2:
>   - Introduce qemu_utimensat()
> 
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

Looks good to me (no strong opinion on the cutils vs oslib-posix that
Jes mentioned).

Acked-by: Chris Wright <chrisw@sous-sol.org>

[Qemu-devel] Re: [PATCH v3] virtio-9p: fix build on !CONFIG_UTIMENSAT

[M. Mohan Kumar]

> This patch introduce a fallback mechanism for old systems that do not
> support utimensat().  This fix build failure with following warnings:
> 
> hw/virtio-9p-local.c: In function 'local_utimensat':
> hw/virtio-9p-local.c:479: warning: implicit declaration of function
> 'utimensat' hw/virtio-9p-local.c:479: warning: nested extern declaration
> of 'utimensat'
> 
> and:
> 
> hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
> hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this
> function) hw/virtio-9p.c:1410: error: (Each undeclared identifier is
> reported only once hw/virtio-9p.c:1410: error: for each function it
> appears in.)
> hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this
> function) hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
> hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this
> function)
> 
> v3:
>   - Use better alternative handling for UTIME_NOW/OMIT
>   - Move qemu_utimensat() to cutils.c
> V2:
>   - Introduce qemu_utimensat()
> 
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

Looks good to me.

Acked-by: M. Mohan Kumar <mohan@in.ibm.com>

Re: [Qemu-devel] [PATCH v3] virtio-9p: fix build on !CONFIG_UTIMENSAT

[Anthony Liguori]

On 11/14/2010 08:15 PM, Hidetoshi Seto wrote:
> This patch introduce a fallback mechanism for old systems that do not
> support utimensat().  This fix build failure with following warnings:
>
> hw/virtio-9p-local.c: In function 'local_utimensat':
> hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
> hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
>
> and:
>
> hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
> hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this function)
> hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once
> hw/virtio-9p.c:1410: error: for each function it appears in.)
> hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this function)
> hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
> hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this function)
>
> v3:
>    - Use better alternative handling for UTIME_NOW/OMIT
>    - Move qemu_utimensat() to cutils.c
> V2:
>    - Introduce qemu_utimensat()
>
> Signed-off-by: Hidetoshi Seto<seto.hidetoshi@jp.fujitsu.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   cutils.c             |   43 +++++++++++++++++++++++++++++++++++++++++++
>   hw/virtio-9p-local.c |    4 ++--
>   qemu-common.h        |   10 ++++++++++
>   3 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 536ee93..3c18941 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -288,3 +288,46 @@ int fcntl_setfl(int fd, int flag)
>   }
>   #endif
>
> +int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
> +                   int flags)
> +{
> +#ifdef CONFIG_UTIMENSAT
> +    return utimensat(dirfd, path, times, flags);
> +#else
> +    /* Fallback: use utimes() instead of utimensat() */
> +    struct timeval tv[2], tv_now;
> +    struct stat st;
> +    int i;
> +
> +    /* happy if special cases */
> +    if (times[0].tv_nsec == UTIME_OMIT&&  times[1].tv_nsec == UTIME_OMIT) {
> +        return 0;
> +    }
> +    if (times[0].tv_nsec == UTIME_NOW&&  times[1].tv_nsec == UTIME_NOW) {
> +        return utimes(path, NULL);
> +    }
> +
> +    /* prepare for hard cases */
> +    if (times[0].tv_nsec == UTIME_NOW || times[1].tv_nsec == UTIME_NOW) {
> +        gettimeofday(&tv_now, NULL);
> +    }
> +    if (times[0].tv_nsec == UTIME_OMIT || times[1].tv_nsec == UTIME_OMIT) {
> +        stat(path,&st);
> +    }
> +
> +    for (i = 0; i<  2; i++) {
> +        if (times[i].tv_nsec == UTIME_NOW) {
> +            tv[i].tv_sec = tv_now.tv_sec;
> +            tv[i].tv_usec = 0;
> +        } else if (times[i].tv_nsec == UTIME_OMIT) {
> +            tv[i].tv_sec = (i == 0) ? st.st_atime : st.st_mtime;
> +            tv[i].tv_usec = 0;
> +        } else {
> +            tv[i].tv_sec = times[i].tv_sec;
> +            tv[i].tv_usec = times[i].tv_nsec / 1000;
> +        }
> +    }
> +
> +    return utimes(path,&tv[0]);
> +#endif
> +}
> diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
> index 0d52020..41603ea 100644
> --- a/hw/virtio-9p-local.c
> +++ b/hw/virtio-9p-local.c
> @@ -480,9 +480,9 @@ static int local_chown(FsContext *fs_ctx, const char *path, FsCred *credp)
>   }
>
>   static int local_utimensat(FsContext *s, const char *path,
> -		       const struct timespec *buf)
> +                           const struct timespec *buf)
>   {
> -    return utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
> +    return qemu_utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
>   }
>
>   static int local_remove(FsContext *ctx, const char *path)
> diff --git a/qemu-common.h b/qemu-common.h
> index 2fbc27f..7fe4c16 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -146,6 +146,16 @@ time_t mktimegm(struct tm *tm);
>   int qemu_fls(int i);
>   int qemu_fdatasync(int fd);
>   int fcntl_setfl(int fd, int flag);
> +#ifndef CONFIG_UTIMENSAT
> +#ifndef UTIME_NOW
> +# define UTIME_NOW     ((1l<<  30) - 1l)
> +#endif
> +#ifndef UTIME_OMIT
> +# define UTIME_OMIT    ((1l<<  30) - 2l)
> +#endif
> +#endif
> +int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
> +    int flags);
>
>   /* path.c */
>   void init_paths(const char *prefix);
>    

Re: [Qemu-devel] [PATCH v3] virtio-9p: fix build on !CONFIG_UTIMENSAT

[Jes Sorensen]

On 11/21/10 16:22, Anthony Liguori wrote:
> On 11/14/2010 08:15 PM, Hidetoshi Seto wrote:
>> This patch introduce a fallback mechanism for old systems that do not
>> support utimensat().  This fix build failure with following warnings:
>>
>> hw/virtio-9p-local.c: In function 'local_utimensat':
>> hw/virtio-9p-local.c:479: warning: implicit declaration of function
>> 'utimensat'
>> hw/virtio-9p-local.c:479: warning: nested extern declaration of
>> 'utimensat'
>>
>> and:
>>
>> hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
>> hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this
>> function)
>> hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported
>> only once
>> hw/virtio-9p.c:1410: error: for each function it appears in.)
>> hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this
>> function)
>> hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
>> hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this
>> function)
>>
>> v3:
>>    - Use better alternative handling for UTIME_NOW/OMIT
>>    - Move qemu_utimensat() to cutils.c
>> V2:
>>    - Introduce qemu_utimensat()
>>
>> Signed-off-by: Hidetoshi Seto<seto.hidetoshi@jp.fujitsu.com>
>>    
> 
> Applied.  Thanks.
> 
> Regards,
> 
> Anthony Liguori

Anthony,

Did you actually apply this one? I don't see it in the git tree.

However if you did, that was a mistake, the qemu_utimensat() should not
have gone into cutils.c as I pointed out earlier, it is inconsistent.

Cheers,
Jes