Re: [Qemu-devel] [PATCH 1/4] bsd-user: Implement new syscall print_sysarch and add strace support

[Peter Maydell]

On 20 June 2014 01:19, Sean Bruno <address@hidden> wrote:
> Signed-off-by: Sean Bruno <address@hidden>
> Signed-off-by: Stacey Son <address@hidden>
> ---
>  bsd-user/freebsd/os-strace.h           | 35 +++++++++++++++
>  bsd-user/freebsd/strace.list           |  2 +-
>  bsd-user/i386/syscall.h                | 21 +++++++++
>  bsd-user/i386/target_arch_sysarch.h    | 78 
> ++++++++++++++++++++++++++++++++++
>  bsd-user/netbsd/os-strace.h            | 37 ++++++++++++++++
>  bsd-user/openbsd/os-strace.h           | 37 ++++++++++++++++
>  bsd-user/qemu.h                        |  3 ++
>  bsd-user/sparc/syscall.h               | 27 +++++++++++-
>  bsd-user/sparc/target_arch_sysarch.h   | 52 +++++++++++++++++++++++
>  bsd-user/sparc64/syscall.h             | 26 +++++++++++-
>  bsd-user/sparc64/target_arch_sysarch.h | 52 +++++++++++++++++++++++
>  bsd-user/strace.c                      | 10 +++++
>  bsd-user/syscall.c                     | 63 +--------------------------
>  bsd-user/x86_64/syscall.h              | 24 ++++++++++-
>  bsd-user/x86_64/target_arch_sysarch.h  | 76 +++++++++++++++++++++++++++++++++
>  15 files changed, 477 insertions(+), 66 deletions(-)
>  create mode 100644 bsd-user/freebsd/os-strace.h
>  create mode 100644 bsd-user/i386/target_arch_sysarch.h
>  create mode 100644 bsd-user/netbsd/os-strace.h
>  create mode 100644 bsd-user/openbsd/os-strace.h
>  create mode 100644 bsd-user/sparc/target_arch_sysarch.h
>  create mode 100644 bsd-user/sparc64/target_arch_sysarch.h
>  create mode 100644 bsd-user/x86_64/target_arch_sysarch.h

I think this would be clearer if you split it in two:
 * code motion to split the syscall into per-OS/per-arch
   subdirectories
 * add support for strace for it

At the moment this is a big patch doing two distinct things,
which is usually a sign it should be split up.

Your commit message is also very brief and inaccurate
(the syscall is not called "print_sysarch" and we already
implemented it, this is just refactoring). Single-line
commit messages should be the exception, not the rule,
and generally only for very simple small patches.

Codewise there are just a few minor things:

> +++ b/bsd-user/netbsd/os-strace.h
> @@ -0,0 +1,37 @@
> +/*
> + *  NetBSD dependent strace print functions
> + *
> + *  Copyright (c) 2014 Sean Bruno <address@hidden>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "target_arch_sysarch.h"    /* architecture dependent functions */
> +
> +
> +static inline void do_os_print_sysarch(const struct syscallname *name,
> +        abi_long arg1, abi_long arg2, abi_long arg3, abi_long arg4,
> +        abi_long arg5, abi_long arg6)
> +{
> +    qemu_log("qemu: Unsupported syscall %s\n", __func__);
> +    return -TARGET_ENOSYS;

Void function attempting to return a value... (the compiler
ought to warn about this I hope).

> +static inline void do_os_print_sysarch(const struct syscallname *name,
> +        abi_long arg1, abi_long arg2, abi_long arg3, abi_long arg4,
> +        abi_long arg5, abi_long arg6)
> +{
> +    qemu_log("qemu: Unsupported syscall %s\n", __func__);
> +    return -TARGET_ENOSYS;
> +}

Ditto.

> +static inline abi_long do_freebsd_arch_sysarch(void *env, int op,
> +        abi_ulong parms)
> +{
> +    int ret = 0;
> +
> +    switch (op) {
> +    case TARGET_SPARC_SIGTRAMP_INSTALL:
> +        /* XXX not currently handled */
> +    case TARGET_SPARC_UTRAP_INSTALL:
> +        /* XXX not currently handled */

If you want to fall through to a following case you should
have a marker comment like this:
    /* fallthrough */

for the benefit of automatic static analysis tools.

> +static inline abi_long do_freebsd_arch_sysarch(void *env, int op,
> +        abi_ulong parms)
> +{
> +    int ret = 0;
> +
> +    switch (op) {
> +    case TARGET_SPARC_SIGTRAMP_INSTALL:
> +        /* XXX not currently handled */
> +    case TARGET_SPARC_UTRAP_INSTALL:
> +        /* XXX not currently handled */
> +    default:
> +        ret = -TARGET_EINVAL;
> +        break;
> +    }

Ditto.

thanks
-- PMM