Re: [Qemu-devel] [PATCH 04/10] linux-user: Sync fcntl.h bits with the kernel

[Peter Maydell]

On 25 July 2012 23:10, Richard Henderson <address@hidden> wrote:
> For each target, only define the bits that appear in
> arch/target/include/asm/fcntl.h.  Mirror the kernel's
> asm-generic layout by handling anything undefined afterward.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  linux-user/syscall_defs.h |  195 +++++++++++++++++++++-----------------------
>  1 files changed, 93 insertions(+), 102 deletions(-)
>
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 02fe4f6..974d6f5 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2012,135 +2012,126 @@ struct target_statfs64 {
>  #define TARGET_F_DUPFD_CLOEXEC (TARGET_F_LINUX_SPECIFIC_BASE + 6)
>  #define TARGET_F_NOTIFY  (TARGET_F_LINUX_SPECIFIC_BASE+2)
>

No entries for TARGET_M68K? I checked and I think that's the
only supported arch which isn't listed here and isn't only using
the asm-generic values. (It's the same 4 defines as TARGET_ARM.)

> -#if defined (TARGET_ARM)
> -#define TARGET_O_ACCMODE          0003
> -#define TARGET_O_RDONLY             00
> -#define TARGET_O_WRONLY             01
> -#define TARGET_O_RDWR               02
> -#define TARGET_O_CREAT            0100 /* not fcntl */
> -#define TARGET_O_EXCL             0200 /* not fcntl */
> -#define TARGET_O_NOCTTY           0400 /* not fcntl */
> -#define TARGET_O_TRUNC           01000 /* not fcntl */
> -#define TARGET_O_APPEND          02000
> -#define TARGET_O_NONBLOCK        04000
> -#define TARGET_O_NDELAY        TARGET_O_NONBLOCK
> -#define TARGET_O_SYNC           010000
> -#define TARGET_FASYNC           020000 /* fcntl, for BSD compatibility */
> +#if defined(TARGET_ALPHA)
> +#define TARGET_O_NONBLOCK          04
> +#define TARGET_O_APPEND            010
> +#define TARGET_O_CREAT           01000 /* not fcntl */
> +#define TARGET_O_TRUNC           02000 /* not fcntl */
> +#define TARGET_O_EXCL            04000 /* not fcntl */
> +#define TARGET_O_NOCTTY         010000 /* not fcntl */
> +#define TARGET_FASYNC          020000 /* fcntl, for BSD compatibility */
> +#define TARGET_O_DSYNC         040000
> +#define TARGET_O_LARGEFILE          0  /* not necessary, always 64-bit */
> +#define TARGET_O_DIRECTORY     0100000 /* must be a directory */
> +#define TARGET_O_NOFOLLOW      0200000 /* don't follow links */
> +#define TARGET_O_DIRECT       02000000 /* direct disk access hint */
> +#define TARGET_O_NOATIME      04000000
> +#define TARGET_O_CLOEXEC     010000000
> +#define TARGET___O_SYNC      020000000
> +#define TARGET_O_PATH        040000000

The other targets were all straightforward to review but I had a
little difficulty with the Alpha bits here.
(1) O_LARGEFILE is 0 here but not in the kernel. Does the "not
necessary" comment here mean "not necessary for QEMU" ?
(ie is the difference because we are taking a different approach to
the kernel's "always set this bit on entry" one ?)

(2) where has the TARGET_FASYNC entry come from?

There are also a couple of hardcoded tabs lurking in some of
these lines which it would be nice to get rid of since we're
changing the code anyway. (checkpatch will tell you where they are.)

> +#elif defined (TARGET_ARM)

Not having the space between 'defined' and '(' would be consistent
with the TARGET_ALPHA one, be consistent with the lion's share of
defined() uses elswhere and also shut checkpatch up :-)

Otherwise good and I've checked all the numbers against the kernel
headers.

-- PMM