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

[PATCH v4] 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)

v4:
  - Use tv_now.tv_usec
  - Rebased on latest qemu.git
v3:
  - Use better alternative handling for UTIME_NOW/OMIT
  - Move qemu_utimensat() to cutils.c
V2:
  - Introduce qemu_utimensat()

Acked-by: Chris Wright <chrisw@sous-sol.org>
Acked-by: M. Mohan Kumar <mohan@in.ibm.com>
Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 cutils.c             |   44 ++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-9p-local.c |    4 ++--
 qemu-common.h        |   10 ++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/cutils.c b/cutils.c
index 28089aa..dbeb3f2 100644
--- a/cutils.c
+++ b/cutils.c
@@ -371,3 +371,47 @@ fail:
 
     return retval;
 }
+
+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 = tv_now.tv_usec;
+        } 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 b3957f1..f0b2c9d 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -150,6 +150,16 @@ int qemu_fls(int i);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
 ssize_t strtosz(const char *nptr, char **end);
+#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

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

[Jes Sorensen]

On 11/18/10 01:41, 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)
> 
> v4:
>   - Use tv_now.tv_usec
>   - Rebased on latest qemu.git
> v3:
>   - Use better alternative handling for UTIME_NOW/OMIT
>   - Move qemu_utimensat() to cutils.c
> V2:
>   - Introduce qemu_utimensat()
> 
> Acked-by: Chris Wright <chrisw@sous-sol.org>
> Acked-by: M. Mohan Kumar <mohan@in.ibm.com>
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

Hi Hidetoshi,

I think the idea of the patch is good, but please move qemu_utimensat()
to oslib-posix.c and provide a wrapper for oslib-win32.c. It is
emulation for a system library function, so it doesn't belong in
cutils.c, but rather in the oslib group.

Thanks,
Jes

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

[Philipp Hahn]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

Hello,

Am Donnerstag 18 November 2010 01:41:39 schrieb Hidetoshi Seto:
> This patch introduce a fallback mechanism for old systems that do not
> support utimensat().  This fix build failure with following warnings:

> +#ifdef CONFIG_UTIMENSAT
> +    return utimensat(dirfd, path, times, flags);
> +#else
> +    /* Fallback: use utimes() instead of utimensat() */

Since we also had a problem with utimestat() some time ago with Samba
<http://lists.samba.org/archive/samba-technical/2010-November/074613.html>
I'd like to comment on that:

Your have to be careful about compile-time-detection and runtime-detection: If 
you later run your utimestat()-enabled binary on an older kernel not 
supporting that syscall, you get -1 as the return-value and errno=ENOSYS. So 
even if you detected utimesatat() during compile-time, please always provide 
a fallback for run-time.
This is less important for people compiling there own version of kvm, but is 
essential for Linux distributions, since people often run newer kvm versions 
on older kernels.

Sincerely
Philipp Hahn
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de   
Univention GmbH        Linux for Your Business        fon: +49 421 22 232- 0
Mary-Somerville-Str.1  28359 Bremen                   fax: +49 421 22 232-99
                                                    http://www.univention.de

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Hidetoshi Seto]

(2010/11/18 17:02), Jes Sorensen wrote:
> On 11/18/10 01:41, 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)
>>
>> v4:
>>   - Use tv_now.tv_usec
>>   - Rebased on latest qemu.git
>> v3:
>>   - Use better alternative handling for UTIME_NOW/OMIT
>>   - Move qemu_utimensat() to cutils.c
>> V2:
>>   - Introduce qemu_utimensat()
>>
>> Acked-by: Chris Wright <chrisw@sous-sol.org>
>> Acked-by: M. Mohan Kumar <mohan@in.ibm.com>
>> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> 
> Hi Hidetoshi,
> 
> I think the idea of the patch is good, but please move qemu_utimensat()
> to oslib-posix.c and provide a wrapper for oslib-win32.c. It is
> emulation for a system library function, so it doesn't belong in
> cutils.c, but rather in the oslib group.

Unfortunately one fact is that I'm not familiar with win32 codes so I don't
have any idea how the wrapper for win32 will be...
If someone could kindly tell me about the win32 part, I could update this
patch to v5, but even though I have no test environment for the new part :-<

Could we wait an incremental patch on this v4?
Can somebody help me?  Volunteers?


Thanks,
H.Seto

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

[Hidetoshi Seto]

(2010/11/18 17:28), Philipp Hahn wrote:
> Hello,
> 
> Am Donnerstag 18 November 2010 01:41:39 schrieb Hidetoshi Seto:
>> This patch introduce a fallback mechanism for old systems that do not
>> support utimensat().  This fix build failure with following warnings:
> 
>> +#ifdef CONFIG_UTIMENSAT
>> +    return utimensat(dirfd, path, times, flags);
>> +#else
>> +    /* Fallback: use utimes() instead of utimensat() */
> 
> Since we also had a problem with utimestat() some time ago with Samba
> <http://lists.samba.org/archive/samba-technical/2010-November/074613.html>
> I'd like to comment on that:
> 
> Your have to be careful about compile-time-detection and runtime-detection: If 
> you later run your utimestat()-enabled binary on an older kernel not 
> supporting that syscall, you get -1 as the return-value and errno=ENOSYS. So 
> even if you detected utimesatat() during compile-time, please always provide 
> a fallback for run-time.
> This is less important for people compiling there own version of kvm, but is 
> essential for Linux distributions, since people often run newer kvm versions 
> on older kernels.

Hum, you have a good point.

Well, then I'll change it like:

-#ifdef CONFIG_UTIMENSAT
-    return utimensat(dirfd, path, times, flags);
-#else
+    {
+        int ret = utimensat(dirfd, path, times, flags);
+        if (ret != -1 || errno != ENOSYS) {
+            return ret;
+        }
+    }


Thanks,
H.Seto

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

[Jes Sorensen]

On 11/18/10 09:48, Hidetoshi Seto wrote:
> (2010/11/18 17:02), Jes Sorensen wrote:
>> Hi Hidetoshi,
>>
>> I think the idea of the patch is good, but please move qemu_utimensat()
>> to oslib-posix.c and provide a wrapper for oslib-win32.c. It is
>> emulation for a system library function, so it doesn't belong in
>> cutils.c, but rather in the oslib group.
> 
> Unfortunately one fact is that I'm not familiar with win32 codes so I don't
> have any idea how the wrapper for win32 will be...
> If someone could kindly tell me about the win32 part, I could update this
> patch to v5, but even though I have no test environment for the new part :-<
> 
> Could we wait an incremental patch on this v4?
> Can somebody help me?  Volunteers?

Hi Hidetoshi,

I don't actually know much about win32 myself, the only thing I do is to
try and cross-compile for it using mingw32 to make sure the build
doesn't break. One option is to leave it open, or put in a dummy wrapper
which asserts in the win32 part of the code, so that someone who is
interested in win32 can fix it up.

That should be pretty easy to do, and I think thats a fine starting point.

Cheers,
Jes

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

[Philipp Hahn]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

Hello,

Am Donnerstag 18 November 2010 10:05:59 schrieb Hidetoshi Seto:
> > Your have to be careful about compile-time-detection and
> > runtime-detection:
...
> -#ifdef CONFIG_UTIMENSAT
> -    return utimensat(dirfd, path, times, flags);
> -#else
> +    {
> +        int ret = utimensat(dirfd, path, times, flags);
> +        if (ret != -1 || errno != ENOSYS) {
> +            return ret;
> +        }
> +    }

You might still want to do the compile-time-check, something like:
#ifdef CONFIG_UTIMENSAT
    {
        int ret = utimensat(dirfd, path, times, flags);
        if (ret != -1 || errno != ENOSYS) {
            return ret;
        }
    }
#endif
// fallback

Sincerely
Philipp Hahn
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de   
Univention GmbH        Linux for Your Business        fon: +49 421 22 232- 0
Mary-Somerville-Str.1  28359 Bremen                   fax: +49 421 22 232-99
                                                    http://www.univention.de

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]