Re: [Qemu-devel] [PATCH 4/4] bsd-user: move arch/OS dependent code out of syscall.c

[Peter Maydell]

On 20 June 2014 01:19, Sean Bruno <address@hidden> wrote:
> From: Stacey Son <address@hidden>
>
> This change moves the system call handler for sysctl(2) and
> sysarch(2) from syscall.c to the OS and arch dependent directories.
> This eliminates many of the #ifdef's in syscall.c.  These system
> call handlers are now located in the host os and target arch
> directories.
>
> Signed-off-by: Sean Bruno <address@hidden>
> Signed-off-by: Stacey Son <address@hidden>
> ---
>  bsd-user/Makefile.objs                  |   2 +-
>  bsd-user/bsdload.c                      | 168 +++++++++++++------

Why are we making lots of changes to bsdload.c then? This
doesn't match up with the commit message...

>  bsd-user/elfload.c                      |   7 +-
>  bsd-user/freebsd/os-strace.h            |   6 -
>  bsd-user/freebsd/os-sys.c               | 285 
> ++++++++++++++++++++++++++++++++
>  bsd-user/freebsd/target_os_stack.h      | 157 ++++++++++++++++++
>  bsd-user/i386/syscall.h                 |   2 +
>  bsd-user/i386/target_arch_sigtramp.h    |  11 ++
>  bsd-user/main.c                         |   2 +-
>  bsd-user/netbsd/os-strace.h             |   8 -
>  bsd-user/netbsd/os-sys.c                |  46 ++++++
>  bsd-user/netbsd/target_os_stack.h       |  33 ++++
>  bsd-user/openbsd/os-strace.h            |   8 -
>  bsd-user/openbsd/os-sys.c               |  46 ++++++
>  bsd-user/openbsd/target_os_stack.h      |  33 ++++
>  bsd-user/qemu.h                         |  28 +++-
>  bsd-user/sparc/syscall.h                |   2 +
>  bsd-user/sparc/target_arch_sigtramp.h   |  11 ++
>  bsd-user/sparc64/syscall.h              |   2 +
>  bsd-user/sparc64/target_arch_sigtramp.h |  11 ++
>  bsd-user/syscall.c                      | 151 ++++-------------
>  bsd-user/x86_64/syscall.h               |   2 +
>  bsd-user/x86_64/target_arch_sigtramp.h  |  11 ++
>  23 files changed, 827 insertions(+), 205 deletions(-)
>  create mode 100644 bsd-user/freebsd/os-sys.c
>  create mode 100644 bsd-user/freebsd/target_os_stack.h
>  create mode 100644 bsd-user/i386/target_arch_sigtramp.h
>  create mode 100644 bsd-user/netbsd/os-sys.c
>  create mode 100644 bsd-user/netbsd/target_os_stack.h
>  create mode 100644 bsd-user/openbsd/os-sys.c
>  create mode 100644 bsd-user/openbsd/target_os_stack.h
>  create mode 100644 bsd-user/sparc/target_arch_sigtramp.h
>  create mode 100644 bsd-user/sparc64/target_arch_sigtramp.h
>  create mode 100644 bsd-user/x86_64/target_arch_sigtramp.h

Again, this patch is doing too much at once. I would
suggest you could split it out into at least:
 * 1 patch for sigtramp related code
 * 1 patch for os_stack related code
 * 1 or more patches for the loader changes
 * 1 patch for sysctl changes (making sure you keep
   "add new functionality" separate from "just moving
   code around"; I'm pretty sure you're adding support for
   new sysctls here, like the HW_MACHINE stuff)
 * either drop whitespace/format changes or put them in
   their own patch, your choice

> diff --git a/bsd-user/freebsd/os-strace.h b/bsd-user/freebsd/os-strace.h
> index c856450..a222f09 100644
> --- a/bsd-user/freebsd/os-strace.h
> +++ b/bsd-user/freebsd/os-strace.h
> @@ -27,9 +27,3 @@ static inline void do_os_print_sysarch(const struct 
> syscallname *name,
>      /* This is arch dependent */
>      do_freebsd_arch_print_sysarch(name, arg1, arg2, arg3, arg4, arg5, arg6);
>  }
> -
> -/* sysarch() is architecture dependent. */
> -abi_long do_bsd_sysarch(void *cpu_env, abi_long arg1, abi_long arg2)
> -{
> -    return do_freebsd_arch_sysarch(cpu_env, arg1, arg2);
> -}

We just added this function in patch 1, why are we removing it again?
Either patch 1 or this one is wrong... There are other bits of
code in this patch touching the sysarch code which should be
in patch 1 as well.

thanks
-- PMM