[Qemu-devel] [PATCH] fcntl64 fix

[Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

[-- Attachment #1.1: Type: text/plain, Size: 337 bytes --]

TARGET_F_*64 should be used instead of F_*64, because on 64-bit host
systems F_GETLK == F_GETLK64(same for SETLK and SETLKW), so we cannot
determinate if it's a long lock or not on a target 32-bit system.
Patch in the attachment.

P.S. Please, review my privious patches, which I have added description
recently. Or should I repost it?


[-- Attachment #1.2: qemu-0.9.0-alt-fcntl64-fix.patch --]
[-- Type: text/plain, Size: 774 bytes --]

diff -uNr qemu-0.9.0.cvs20070304.orig/linux-user/syscall.c qemu-0.9.0.cvs20070304/linux-user/syscall.c
--- qemu-0.9.0.cvs20070304.orig/linux-user/syscall.c	2007-03-09 20:08:59 +0200
+++ qemu-0.9.0.cvs20070304/linux-user/syscall.c	2007-03-09 20:09:54 +0200
@@ -4063,7 +4063,7 @@
 #endif
 
         switch(arg2) {
-        case F_GETLK64:
+        case TARGET_F_GETLK64:
             ret = get_errno(fcntl(arg1, arg2, &fl));
 	    if (ret == 0) {
 #ifdef TARGET_ARM
@@ -4089,8 +4089,8 @@
 	    }
 	    break;
 
-        case F_SETLK64:
-        case F_SETLKW64:
+        case TARGET_F_SETLK64:
+        case TARGET_F_SETLKW64:
 #ifdef TARGET_ARM
             if (((CPUARMState *)cpu_env)->eabi) {
                 lock_user_struct(target_efl, arg3, 1);

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Thiemo Seufer]

Kirill A. Shutemov wrote:
> TARGET_F_*64 should be used instead of F_*64, because on 64-bit host
> systems F_GETLK == F_GETLK64(same for SETLK and SETLKW), so we cannot
> determinate if it's a long lock or not on a target 32-bit system.
> Patch in the attachment.
> 
> P.S. Please, review my privious patches, which I have added description
> recently. Or should I repost it?
> 

> diff -uNr qemu-0.9.0.cvs20070304.orig/linux-user/syscall.c qemu-0.9.0.cvs20070304/linux-user/syscall.c
> --- qemu-0.9.0.cvs20070304.orig/linux-user/syscall.c	2007-03-09 20:08:59 +0200
> +++ qemu-0.9.0.cvs20070304/linux-user/syscall.c	2007-03-09 20:09:54 +0200
> @@ -4063,7 +4063,7 @@
>  #endif
>  
>          switch(arg2) {
> -        case F_GETLK64:
> +        case TARGET_F_GETLK64:
>              ret = get_errno(fcntl(arg1, arg2, &fl));

This changes the bug from checking the wrong flag to (potentially)
passing down the wrong flag...

>  	    if (ret == 0) {
>  #ifdef TARGET_ARM
> @@ -4089,8 +4089,8 @@
>  	    }
>  	    break;
>  
> -        case F_SETLK64:
> -        case F_SETLKW64:
> +        case TARGET_F_SETLK64:
> +        case TARGET_F_SETLKW64:
>  #ifdef TARGET_ARM
>              if (((CPUARMState *)cpu_env)->eabi) {
>                  lock_user_struct(target_efl, arg3, 1);

Likewise here. We should always check TARGET_* flags and pass down the
corresponding host flag.


Thiemo

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Stuart Anderson]

My initial fix was before I started using LTP, and just took care of a
single case that was holding me up. Now I have run the fcntl tests in
LTP on ARM (both oABI and EABI) and there are a lot of failures indicating
that there is a lot more work to be done yet on fcntl().

I'll take a look into it, and probably resubmit a bigger patch later.



                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Stuart Anderson]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 975 bytes --]

On Mon, 19 Mar 2007, Stuart Anderson wrote:

>
> My initial fix was before I started using LTP, and just took care of a
> single case that was holding me up. Now I have run the fcntl tests in
> LTP on ARM (both oABI and EABI) and there are a lot of failures indicating
> that there is a lot more work to be done yet on fcntl().
>
> I'll take a look into it, and probably resubmit a bigger patch later.

One more small fix to repack a structure from taget -> host before using
it clears up most of the fcntl() errors that showed up in LTP. This is
one of those that probably doesn't happen when runngin 32 on 32, but I'm
running 32 on 64.



                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

[-- Attachment #2: fcntl fix --]
[-- Type: TEXT/PLAIN, Size: 662 bytes --]

--- linux-user/syscall.c.orig	2007-03-20 01:25:39.000000000 -0400
+++ linux-user/syscall.c	2007-03-20 02:32:39.000000000 -0400
@@ -2107,6 +2107,13 @@
 
     switch(cmd) {
     case TARGET_F_GETLK:
+        lock_user_struct(target_fl, arg, 1);
+        fl.l_type = tswap16(target_fl->l_type);
+        fl.l_whence = tswap16(target_fl->l_whence);
+        fl.l_start = tswapl(target_fl->l_start);
+        fl.l_len = tswapl(target_fl->l_len);
+        fl.l_pid = tswapl(target_fl->l_pid);
+        unlock_user_struct(target_fl, arg, 0);
         ret = fcntl(fd, cmd, &fl);
         if (ret == 0) {
             lock_user_struct(target_fl, arg, 0);

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

[-- Attachment #1.1: Type: text/plain, Size: 1597 bytes --]

Yep. You're right. Fixed patch in the attachment.

On [Mon, 19.03.2007 17:12], Thiemo Seufer wrote:
> Kirill A. Shutemov wrote:
> > TARGET_F_*64 should be used instead of F_*64, because on 64-bit host
> > systems F_GETLK == F_GETLK64(same for SETLK and SETLKW), so we cannot
> > determinate if it's a long lock or not on a target 32-bit system.
> > Patch in the attachment.
> > 
> > P.S. Please, review my privious patches, which I have added description
> > recently. Or should I repost it?
> > 
> 
> > diff -uNr qemu-0.9.0.cvs20070304.orig/linux-user/syscall.c qemu-0.9.0.cvs20070304/linux-user/syscall.c
> > --- qemu-0.9.0.cvs20070304.orig/linux-user/syscall.c	2007-03-09 20:08:59 +0200
> > +++ qemu-0.9.0.cvs20070304/linux-user/syscall.c	2007-03-09 20:09:54 +0200
> > @@ -4063,7 +4063,7 @@
> >  #endif
> >  
> >          switch(arg2) {
> > -        case F_GETLK64:
> > +        case TARGET_F_GETLK64:
> >              ret = get_errno(fcntl(arg1, arg2, &fl));
> 
> This changes the bug from checking the wrong flag to (potentially)
> passing down the wrong flag...
> 
> >  	    if (ret == 0) {
> >  #ifdef TARGET_ARM
> > @@ -4089,8 +4089,8 @@
> >  	    }
> >  	    break;
> >  
> > -        case F_SETLK64:
> > -        case F_SETLKW64:
> > +        case TARGET_F_SETLK64:
> > +        case TARGET_F_SETLKW64:
> >  #ifdef TARGET_ARM
> >              if (((CPUARMState *)cpu_env)->eabi) {
> >                  lock_user_struct(target_efl, arg3, 1);
> 
> Likewise here. We should always check TARGET_* flags and pass down the
> corresponding host flag.

[-- Attachment #1.2: qemu-0.9.0-alt-fcntl64-fix.patch --]
[-- Type: text/plain, Size: 1820 bytes --]

diff -uNr qemu-0.9.0.cvs20070320.orig/linux-user/syscall.c qemu-0.9.0.cvs20070320/linux-user/syscall.c
--- qemu-0.9.0.cvs20070320.orig/linux-user/syscall.c	2007-03-20 13:26:04 +0200
+++ qemu-0.9.0.cvs20070320/linux-user/syscall.c	2007-03-20 13:34:09 +0200
@@ -4058,15 +4058,27 @@
 #if TARGET_LONG_BITS == 32
     case TARGET_NR_fcntl64:
     {
+	int cmd;
 	struct flock64 fl;
 	struct target_flock64 *target_fl;
 #ifdef TARGET_ARM
 	struct target_eabi_flock64 *target_efl;
 #endif
 
+        switch(arg2){
+        case TARGET_F_GETLK64:
+            cmd = F_GETLK64;
+        case TARGET_F_SETLK64:
+            cmd = F_SETLK64;
+        case TARGET_F_SETLKW64:
+            cmd = F_SETLK64;
+        default:
+            cmd = arg2;
+        }
+
         switch(arg2) {
-        case F_GETLK64:
-            ret = get_errno(fcntl(arg1, arg2, &fl));
+        case TARGET_F_GETLK64:
+            ret = get_errno(fcntl(arg1, cmd, &fl));
 	    if (ret == 0) {
 #ifdef TARGET_ARM
                 if (((CPUARMState *)cpu_env)->eabi) {
@@ -4091,8 +4103,8 @@
 	    }
 	    break;
 
-        case F_SETLK64:
-        case F_SETLKW64:
+        case TARGET_F_SETLK64:
+        case TARGET_F_SETLKW64:
 #ifdef TARGET_ARM
             if (((CPUARMState *)cpu_env)->eabi) {
                 lock_user_struct(target_efl, arg3, 1);
@@ -4113,10 +4125,10 @@
                 fl.l_pid = tswapl(target_fl->l_pid);
                 unlock_user_struct(target_fl, arg3, 0);
             }
-            ret = get_errno(fcntl(arg1, arg2, &fl));
+            ret = get_errno(fcntl(arg1, cmd, &fl));
 	    break;
         default:
-            ret = get_errno(do_fcntl(arg1, arg2, arg3));
+            ret = get_errno(do_fcntl(arg1, cmd, arg3));
             break;
         }
 	break;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

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

On [Tue, 20.03.2007 09:55], Stuart Anderson wrote:
> 
> On Tue, 20 Mar 2007, Kirill A. Shutemov wrote:
> 
> >Yep. You're right. Fixed patch in the attachment.
> 
> Kiril,
>  	What 32 bit host and 64 bit host are you using? I'm working on
> arm on x86_64, and I'm starting to think that perhaps all of the different
> parts of the fix are needed to ensure it works correctly on all target/host
> combinations.
> 

I'm using arm(little-endian) on x86_64.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Stuart Anderson]

On Tue, 20 Mar 2007, Kirill A. Shutemov wrote:

>> Kiril,
>>  	What 32 bit host and 64 bit host are you using? I'm working on
>> arm on x86_64, and I'm starting to think that perhaps all of the different
>> parts of the fix are needed to ensure it works correctly on all target/host
>> combinations.
>>
>
> I'm using arm(little-endian) on x86_64.

That blows that theory 8-).

What are you using as a test app? I think that remapping the constants
is needed, but I'm just curious how we seem to be coming up with different
parts of the fix when we have the same target/host combination.



                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

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

On [Tue, 20.03.2007 12:53], Stuart Anderson wrote:
> On Tue, 20 Mar 2007, Kirill A. Shutemov wrote:
> 
> >>Kiril,
> >> 	What 32 bit host and 64 bit host are you using? I'm working on
> >>arm on x86_64, and I'm starting to think that perhaps all of the different
> >>parts of the fix are needed to ensure it works correctly on all 
> >>target/host
> >>combinations.
> >>
> >
> >I'm using arm(little-endian) on x86_64.
> 
> That blows that theory 8-).
>

:)

> 
> What are you using as a test app?

I got error when runing Debian's apt-get and tried to fix it.

> I think that remapping the constants
> is needed, but I'm just curious how we seem to be coming up with different
> parts of the fix when we have the same target/host combination.

I'm not sure that I understand you...


-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Stuart Anderson]

On Tue, 20 Mar 2007, Kirill A. Shutemov wrote:

>> What are you using as a test app?
>
> I got error when runing Debian's apt-get and tried to fix it.

OK, that's what got me started on this one, but I switched to using the
ltp-kernel-test package for a more comprehensive set of tests once I got
past that first eabi structure change.

>> I think that remapping the constants
>> is needed, but I'm just curious how we seem to be coming up with different
>> parts of the fix when we have the same target/host combination.
>
> I'm not sure that I understand you...

On the arm/x86_64 combination, I think the host & target cmd values are
the same, so the remapping is a noop. It may be needed for other
combinations though. Some architectures have very different values for
constants like this in their ABI.

I was trying to understand how your fix made apt-get/dpkg happy, or if
you were just using a different app that was hitting a different case
for fcntl().


                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

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

On [Tue, 20.03.2007 14:03], Stuart Anderson wrote:
> On Tue, 20 Mar 2007, Kirill A. Shutemov wrote:
> 
> >>What are you using as a test app?
> >
> >I got error when runing Debian's apt-get and tried to fix it.
> 
> OK, that's what got me started on this one, but I switched to using the
> ltp-kernel-test package for a more comprehensive set of tests once I got
> past that first eabi structure change.
> 
> >>I think that remapping the constants
> >>is needed, but I'm just curious how we seem to be coming up with different
> >>parts of the fix when we have the same target/host combination.
> >
> >I'm not sure that I understand you...
> 
> On the arm/x86_64 combination, I think the host & target cmd values are
> the same, so the remapping is a noop.

No. Remap is needed:

$ uname -m; echo -e '#include <fcntl.h>\nF_GETLK64' | cpp | tail -1
x86_64
5

$ uname -m; echo -e '#include <fcntl.h>\nF_GETLK64' | cpp | tail -1
armv5l
12

Same for F_SETLK64 and F_SETLKW64.

> It may be needed for other
> combinations though. Some architectures have very different values for
> constants like this in their ABI.
> 
> I was trying to understand how your fix made apt-get/dpkg happy, or if
> you were just using a different app that was hitting a different case
> for fcntl().
> 
> 
>                                 Stuart
> 
> Stuart R. Anderson                               anderson@netsweng.com
> Network & Software Engineering                   http://www.netsweng.com/
> 1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
>                                                  BD03 0A62 E534 37A7 9149
> 
> 
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
> 

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Stuart Anderson]

On Tue, 20 Mar 2007, Kirill A. Shutemov wrote:

> No. Remap is needed:
>
> $ uname -m; echo -e '#include <fcntl.h>\nF_GETLK64' | cpp | tail -1
> x86_64
> 5
>
> $ uname -m; echo -e '#include <fcntl.h>\nF_GETLK64' | cpp | tail -1
> armv5l
> 12
>
> Same for F_SETLK64 and F_SETLKW64.

You are right. I had previously printed out the non-64 versions, and
they are the same, but we not using those.

TARGET_F_GETLK 5
F_GETLK 5
TARGET_F_GETLK64 12
F_GETLK64 5

My confusion...



                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Stuart Anderson]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2297 bytes --]


OK, I think I finally have it all sorted out. Sorry if I sounded dense
along the way.. there were multiple variable, which increases the number
of possible combinations quickly.

The patch from Kirill is needed, and makes things better. One thing I
notice with it is that we now handle TARGET_F_GETLK64 in two places,
first in the case for TARGET_NR_fcntl64 (around line 4300), and then
again in do_fcntl(), which is called in the default case of the first
location. Once difference between the two locations is wether or not
the case for EABI is handled.

In addition to Kirill's patch, my original patch for target_eabi_flock64
is still needed as well as an expanded version of the revised patch I
sent later that does target->host strcture mapping for the F_GETLK*
cases.

I have used the fcntl test sets out of the Linux Test Projects to
measure with an without the different parts of these patches. With
the entire set (Which is attached), 16 of the 18 test sets pass
completely, and a significant portion of test14 (one of the two that
don't pass completely) passes as well. The tests in test14 that fail
may be do to a problem with a syscall other than fcntl(), but I haven't
completely resulved it yet. Without my portion of the patch, the results
are much worse (maybe half-ish are passing).

There is something interesting about test18 (the other one that doesn't
pass). It intentionally passes in a bad value (-1) as the 3rd argument
to fcntl(). It is testing wether it will get EFAULT. With these fixes,
qemu will SEGV as it tries to convert the struct flock (or struct
flock64) from target->host, and encounters the bad address that was
passed in. The initial SEGV is caught, but the handler for it then
SEGVs again. Ideally, we could detect that we are inside an emulated
system call, and be able to just return the EFAULT.

I ran the LTP tests for both old ABI and EABI, and got the same results.


Attached is the combined patch for fcntl().




                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

[-- Attachment #2: fcntl() combined patch --]
[-- Type: TEXT/PLAIN, Size: 4205 bytes --]

--- linux-user/syscall_defs.h.orig	2007-02-23 15:44:47.000000000 -0500
+++ linux-user/syscall_defs.h	2007-02-23 15:44:26.000000000 -0500
@@ -1414,7 +1414,9 @@
 struct target_eabi_flock64 {
 	short  l_type;
 	short  l_whence;
+#if HOST_LONG_BITS == 32
         int __pad;
+#endif
 	unsigned long long l_start;
 	unsigned long long l_len;
 	int  l_pid;
Index: linux-user/syscall.c
===================================================================
--- linux-user/syscall.c.orig	2007-03-20 16:19:11.000000000 -0400
+++ linux-user/syscall.c	2007-03-20 17:04:40.000000000 -0400
@@ -2107,6 +2107,13 @@
 
     switch(cmd) {
     case TARGET_F_GETLK:
+        lock_user_struct(target_fl, arg, 1);
+        fl.l_type = tswap16(target_fl->l_type);
+        fl.l_whence = tswap16(target_fl->l_whence);
+        fl.l_start = tswapl(target_fl->l_start);
+        fl.l_len = tswapl(target_fl->l_len);
+        fl.l_pid = tswapl(target_fl->l_pid);
+        unlock_user_struct(target_fl, arg, 0);
         ret = fcntl(fd, cmd, &fl);
         if (ret == 0) {
             lock_user_struct(target_fl, arg, 0);
@@ -2132,6 +2139,13 @@
         break;
         
     case TARGET_F_GETLK64:
+        lock_user_struct(target_fl64, arg, 1);
+        fl64.l_type = tswap16(target_fl64->l_type) >> 1;
+        fl64.l_whence = tswap16(target_fl64->l_whence);
+        fl64.l_start = tswapl(target_fl64->l_start);
+        fl64.l_len = tswapl(target_fl64->l_len);
+        fl64.l_pid = tswap16(target_fl64->l_pid);
+        unlock_user_struct(target_fl64, arg, 0);
         ret = fcntl(fd, cmd >> 1, &fl64);
         if (ret == 0) {
             lock_user_struct(target_fl64, arg, 0);
@@ -4201,15 +4215,47 @@
 #if TARGET_LONG_BITS == 32
     case TARGET_NR_fcntl64:
     {
+	int cmd;
 	struct flock64 fl;
 	struct target_flock64 *target_fl;
 #ifdef TARGET_ARM
 	struct target_eabi_flock64 *target_efl;
 #endif
 
+       switch(arg2){
+       case TARGET_F_GETLK64:
+           cmd = F_GETLK64;
+       case TARGET_F_SETLK64:
+           cmd = F_SETLK64;
+       case TARGET_F_SETLKW64:
+           cmd = F_SETLKW64;
+       default:
+           cmd = arg2;
+       }
+
         switch(arg2) {
-        case F_GETLK64:
-            ret = get_errno(fcntl(arg1, arg2, &fl));
+        case TARGET_F_GETLK64:
+#ifdef TARGET_ARM
+            if (((CPUARMState *)cpu_env)->eabi) {
+                lock_user_struct(target_efl, arg3, 1);
+                fl.l_type = tswap16(target_efl->l_type);
+                fl.l_whence = tswap16(target_efl->l_whence);
+                fl.l_start = tswap64(target_efl->l_start);
+                fl.l_len = tswap64(target_efl->l_len);
+                fl.l_pid = tswapl(target_efl->l_pid);
+                unlock_user_struct(target_efl, arg3, 0);
+            } else
+#endif
+            {
+                lock_user_struct(target_fl, arg3, 1);
+                fl.l_type = tswap16(target_fl->l_type);
+                fl.l_whence = tswap16(target_fl->l_whence);
+                fl.l_start = tswap64(target_fl->l_start);
+                fl.l_len = tswap64(target_fl->l_len);
+                fl.l_pid = tswapl(target_fl->l_pid);
+                unlock_user_struct(target_fl, arg3, 0);
+            }
+            ret = get_errno(fcntl(arg1, cmd, &fl));
 	    if (ret == 0) {
 #ifdef TARGET_ARM
                 if (((CPUARMState *)cpu_env)->eabi) {
@@ -4234,8 +4280,8 @@
 	    }
 	    break;
 
-        case F_SETLK64:
-        case F_SETLKW64:
+        case TARGET_F_SETLK64:
+        case TARGET_F_SETLKW64:
 #ifdef TARGET_ARM
             if (((CPUARMState *)cpu_env)->eabi) {
                 lock_user_struct(target_efl, arg3, 1);
@@ -4256,10 +4302,10 @@
                 fl.l_pid = tswapl(target_fl->l_pid);
                 unlock_user_struct(target_fl, arg3, 0);
             }
-            ret = get_errno(fcntl(arg1, arg2, &fl));
+            ret = get_errno(fcntl(arg1, cmd, &fl));
 	    break;
         default:
-            ret = get_errno(do_fcntl(arg1, arg2, arg3));
+            ret = get_errno(do_fcntl(arg1, cmd, arg3));
             break;
         }
 	break;

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Paul Brook]

>  struct target_eabi_flock64 {
>  	short  l_type;
>  	short  l_whence;
> +#if HOST_LONG_BITS == 32
>          int __pad;
> +#endif

This change is definitely wrong. This is a target structure, and is packed, so 
should be independent of the host.

Paul

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Thiemo Seufer]

Stuart Anderson wrote:
[snip]
> --- linux-user/syscall_defs.h.orig	2007-02-23 15:44:47.000000000 -0500
> +++ linux-user/syscall_defs.h	2007-02-23 15:44:26.000000000 -0500
> @@ -1414,7 +1414,9 @@
>  struct target_eabi_flock64 {
>  	short  l_type;
>  	short  l_whence;
> +#if HOST_LONG_BITS == 32
>          int __pad;
> +#endif

Still, this part makes no sense to me since it is in a packed struct.
Can you explain why this works better for you?


Thiemo

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

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

On [Tue, 20.03.2007 21:47], Thiemo Seufer wrote:
> Stuart Anderson wrote:
> [snip]
> > --- linux-user/syscall_defs.h.orig	2007-02-23 15:44:47.000000000 -0500
> > +++ linux-user/syscall_defs.h	2007-02-23 15:44:26.000000000 -0500
> > @@ -1414,7 +1414,9 @@
> >  struct target_eabi_flock64 {
> >  	short  l_type;
> >  	short  l_whence;
> > +#if HOST_LONG_BITS == 32
> >          int __pad;
> > +#endif
> 
> Still, this part makes no sense to me since it is in a packed struct.
> Can you explain why this works better for you?

Primarily, I also thought that problem is in padding, because, without the
patch F_GETLK, on 32-bit target recognises as F_GETLK64 on 64-bit host. It's
happen because on 64-bit host and 32-bit target F_GETLK == F_GETLK64 == 
TARGET_F_GETLK. So if you're using qemu-arm on 64-bit host and target eabi 
program calls fcntl(fd,F_GETLK,...), target_eabi_flock64 will be used by 
mistake. Disabling padding can helps in some trivial cases to pass 
pseudo-correct args to fcntl.

Stuart, am I right?

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

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

On [Tue, 20.03.2007 21:47], Thiemo Seufer wrote:
> Stuart Anderson wrote:
> [snip]
> > --- linux-user/syscall_defs.h.orig	2007-02-23 15:44:47.000000000 -0500
> > +++ linux-user/syscall_defs.h	2007-02-23 15:44:26.000000000 -0500
> > @@ -1414,7 +1414,9 @@
> >  struct target_eabi_flock64 {
> >  	short  l_type;
> >  	short  l_whence;
> > +#if HOST_LONG_BITS == 32
> >          int __pad;
> > +#endif
> 
> Still, this part makes no sense to me since it is in a packed struct.
> Can you explain why this works better for you?

Primarily, I also thought that problem is in padding, because, without the
patch F_GETLK, on 32-bit target recognises as F_GETLK64 on 64-bit host.
It's happen because on 64-bit host and 32-bit target F_GETLK == F_GETLK64 ==
TARGET_F_GETLK. So if you're using qemu-arm on 64-bit host and a target eabi
program calls fcntl(fd,F_GETLK,...), target_eabi_flock64 will be used by
mistake. Disabling padding can helps in some trivial cases to pass
pseudo-correct args to fcntl. I guess this part of patch wrong.

Stuart, am I right?

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Stuart Anderson]

On Wed, 21 Mar 2007, Kirill A. Shutemov wrote:

> Primarily, I also thought that problem is in padding, because, without the
> patch F_GETLK, on 32-bit target recognises as F_GETLK64 on 64-bit host.
> It's happen because on 64-bit host and 32-bit target F_GETLK == F_GETLK64 ==
> TARGET_F_GETLK. So if you're using qemu-arm on 64-bit host and a target eabi
> program calls fcntl(fd,F_GETLK,...), target_eabi_flock64 will be used by
> mistake. Disabling padding can helps in some trivial cases to pass
> pseudo-correct args to fcntl. I guess this part of patch wrong.
>
> Stuart, am I right?

Yes, this is a good summary of the trap I initially fell into.


                                   Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                    BD03 0A62 E534 37A7 9149

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Stuart Anderson]

On Tue, 20 Mar 2007, Thiemo Seufer wrote:

> Still, this part makes no sense to me since it is in a packed struct.
> Can you explain why this works better for you?

It worked better, in that it fixed a problem that let me continue on to
fix other issues. After revisiting fcntl() and coming up with the more
comprehensive patch, this change now seems to have no effect, so I
must conceed that it is not needed, please drop that part of the patch.

Now that the dust has settled, I see where the change is probably a
no-op anyway. A quick little test program indicates that on x86_64,
l_start will have an offset of 8 wether the structure is packed or not,
and wether the __pad member is present or not. The unsigned long long is
always going to be aligned to a 8 byte boundary.

                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

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

On [Tue, 20.03.2007 19:05], Stuart Anderson wrote:
> Now that the dust has settled, I see where the change is probably a
> no-op anyway. A quick little test program indicates that on x86_64,
> l_start will have an offset of 8 wether the structure is packed or not,
> and wether the __pad member is present or not. The unsigned long long is
> always going to be aligned to a 8 byte boundary.

It architecture specific I think.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

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

On [Tue, 20.03.2007 19:05], Stuart Anderson wrote:
> Now that the dust has settled, I see where the change is probably a
> no-op anyway. A quick little test program indicates that on x86_64,
> l_start will have an offset of 8 wether the structure is packed or not,
> and wether the __pad member is present or not. The unsigned long long is
> always going to be aligned to a 8 byte boundary.

Its' architecture specific I think.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Paul Brook]

> Now that the dust has settled, I see where the change is probably a
> no-op anyway. A quick little test program indicates that on x86_64,
> l_start will have an offset of 8 wether the structure is packed or not,
> and wether the __pad member is present or not. The unsigned long long is
> always going to be aligned to a 8 byte boundary.

The __pad member is essential. Your logic is wrong is two ways:

a) The struct is packed. This overrides normal alignment and ensures the 
structure contains no padding.
b) long long has whatever alignment the host feels like giving it. There's no 
guarantee it's going to be 8 byte aligned. 

Paul

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Stuart Anderson]

On Tue, 20 Mar 2007, Paul Brook wrote:

>> Now that the dust has settled, I see where the change is probably a
>> no-op anyway. A quick little test program indicates that on x86_64,
>> l_start will have an offset of 8 wether the structure is packed or not,
>> and wether the __pad member is present or not. The unsigned long long is
>> always going to be aligned to a 8 byte boundary.
>
> The __pad member is essential. Your logic is wrong is two ways:
>
> a) The struct is packed. This overrides normal alignment and ensures the
> structure contains no padding.

And in this case, it does remove some tail padding at the end of the
structure.

> b) long long has whatever alignment the host feels like giving it. There's no
> guarantee it's going to be 8 byte aligned.

No there isn't. This was just an observation of what occurs when
building a simple test case on x86_64.


                                  Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                   BD03 0A62 E534 37A7 9149

Re: [Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

[-- Attachment #1.1: Type: text/plain, Size: 310 bytes --]

On [Tue, 20.03.2007 13:42], Kirill A. Shutemov wrote:
> Yep. You're right. Fixed patch in the attachment.
> 

Patch that fixes dummy bug in the attachment. Sorry.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #1.2: fcntl64_ugly_bug.patch --]
[-- Type: text/plain, Size: 582 bytes --]

diff --git a/qemu/linux-user/syscall.c b/qemu/linux-user/syscall.c
index 5ff364d..a23f3f7 100644
--- a/qemu/linux-user/syscall.c
+++ b/qemu/linux-user/syscall.c
@@ -3882,10 +3882,13 @@ long do_syscall(void *cpu_env, int num, long arg1, long arg2, long arg3,
         switch(arg2){
         case TARGET_F_GETLK64:
             cmd = F_GETLK64;
+            break;
         case TARGET_F_SETLK64:
             cmd = F_SETLK64;
+            break;
         case TARGET_F_SETLKW64:
             cmd = F_SETLK64;
+            break;
         default:
             cmd = arg2;
         }

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[Qemu-devel] [PATCH] fcntl64 fix

[Kirill A. Shutemov]

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

TARGET_F_*64 should be used instead of F_*64, because on 64-bit host systems
F_GETLK == F_GETLK64(same for SETLK and SETLKW), so we cannot determinate if 
it's a long lock or not on a target system.
Patch in the attachment.

P.S. Please, review my privious patches, which I have added description
recently. Or should I repost it?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]