Re: [Qemu-devel] [PATCH v3 03/19] bsd-user: move strace OS/arch dependent code to host/arch dirs

[Peter Maydell]

On 17 December 2013 11:52, Stacey Son <address@hidden> wrote:
> This change moves host OS and arch dependent code for the sysarch
> system call related to the -strace functionality into the
> appropriate host OS and target arch directories.

This patch seems to be trying to do too much at once.

Before this patch is applied, bsd-user supports
just i386, x86_64, sparc, sparc64. You should have
a patch which refactors the code for those targets
to move them into the per-target/per-host-os
directories. Then once you've done that (and all
the other refactoring patches), have patches at
the very end of the series which simply add new arm
and mips support in the correct places.


> --- a/bsd-user/freebsd/strace.list
> +++ b/bsd-user/freebsd/strace.list
> @@ -1,7 +1,38 @@
> +/*
> + *  FreeBSD strace list
> + *
> + *
> + *  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/>.
> + */
> +
> +{ TARGET_FREEBSD_NR___acl_aclcheck_fd, "__acl_get_fd", "%s(%d, %d, %#x)", 
> NULL, NULL },
> +{ TARGET_FREEBSD_NR___acl_aclcheck_file, "__acl_get_file", "%s(\"%s\", %d, 
> %#x)", NULL, NULL },
> +{ TARGET_FREEBSD_NR___acl_aclcheck_link, "__acl_get_link", "%s(\"%s\", %d, 
> %#x)", NULL, NULL },
> +{ TARGET_FREEBSD_NR___acl_delete_fd, "__acl_delete_fd", "%s(%d, %d)", NULL, 
> NULL },
> +{ TARGET_FREEBSD_NR___acl_delete_file, "__acl_delete_file", "%s(\"%s\", 
> %d)", NULL, NULL },
> +{ TARGET_FREEBSD_NR___acl_delete_link, "__acl_delete_link", "%s(\"%s\", 
> %d)", NULL, NULL },
> +{ TARGET_FREEBSD_NR___acl_get_fd, "__acl_get_fd", "%s(\"%s\", %d, %#x)", 
> NULL, NULL },
> +{ TARGET_FREEBSD_NR___acl_get_file, "__acl_get_file", "%s(\"%s\", %d, %#x)", 
> NULL, NULL },
> +{ TARGET_FREEBSD_NR___acl_get_link, "__acl_get_link", "%s(\"%s\", %d, %#x)", 
> NULL, NULL },
> +{ TARGET_FREEBSD_NR___acl_set_fd, "__acl_get_fd", "%s(\"%s\", %d, %#x)", 
> NULL, NULL },
> +{ TARGET_FREEBSD_NR___acl_set_file, "__acl_get_file", "%s(\"%s\", %d, %#x)", 
> NULL, NULL },
> +{ TARGET_FREEBSD_NR___acl_set_link, "__acl_get_link", "%s(\"%s\", %d, %#x)", 
> NULL, NULL },
>  { TARGET_FREEBSD_NR___getcwd, "__getcwd", NULL, NULL, NULL },
>  { TARGET_FREEBSD_NR___semctl, "__semctl", NULL, NULL, NULL },
>  { TARGET_FREEBSD_NR___syscall, "__syscall", NULL, NULL, NULL },
> -{ TARGET_FREEBSD_NR___sysctl, "__sysctl", NULL, NULL, NULL },
> +{ TARGET_FREEBSD_NR___sysctl, "__sysctl", NULL, print_sysctl, NULL },
> +{ TARGET_FREEBSD_NR__umtx_op, "_umtx_op", "%s(%#x, %d, %d, %#x, %#x)", NULL, 
> NULL },

This hunk seems to just be adding strace support for a
lot of new syscalls. It ought to be in its own patch,
not in this one.
> --- /dev/null
> +++ b/bsd-user/i386/target_arch_sysarch.h
> @@ -0,0 +1,78 @@
> +/*
> + *  i386 sysarch system call emulation
> + *
> + *  Copyright (c) 2013 Stacey D. Son
> + *
> + *  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/>.
> + */
> +
> +#ifndef __ARCH_SYSARCH_H_
> +#define __ARCH_SYSARCH_H_
> +
> +#include "syscall.h"
> +
> +static inline abi_long do_freebsd_arch_sysarch(CPUX86State *env, int op,
> +        abi_ulong parms)
> +{
> +    abi_long ret = 0;
> +    abi_ulong val;
> +    int idx;
> +
> +    switch (op) {
> +    case TARGET_FREEBSD_I386_SET_GSBASE:
> +    case TARGET_FREEBSD_I386_SET_FSBASE:
> +        if (op == TARGET_FREEBSD_I386_SET_GSBASE) {
> +            idx = R_GS;
> +        } else {
> +            idx = R_FS;
> +        }
> +        if (get_user(val, parms, abi_ulong)) {
> +            return -TARGET_EFAULT;
> +        }
> +        cpu_x86_load_seg(env, idx, 0);
> +        env->segs[idx].base = val;
> +        break;
> +
> +    case TARGET_FREEBSD_I386_GET_GSBASE:
> +    case TARGET_FREEBSD_I386_GET_FSBASE:
> +        if (op == TARGET_FREEBSD_I386_GET_GSBASE) {
> +            idx = R_GS;
> +        } else {
> +            idx = R_FS;
> +        }
> +        val = env->segs[idx].base;
> +        if (put_user(val, parms, abi_ulong)) {
> +            return -TARGET_EFAULT;
> +        }
> +        break;
> +
> +    /* XXX handle the others... */
> +    default:
> +        ret = -TARGET_EINVAL;
> +        break;
> +    }
> +    return ret;
> +}

This patch adds this function, but the old version is
still in syscalls.c (it doesn't get deleted until patch 5).
This patch should be doing the refactoring, which means it
should add the code in the new correct location and also
delete it from the old.

(The patchstack should also compile and work at every
point along it, so git bisect works. If you haven't
tested that, you should do so.)

>  /*
>   * The public interface to this module.
>   */
> -void
> -print_freebsd_syscall(int num,
> -                      abi_long arg1, abi_long arg2, abi_long arg3,
> -                      abi_long arg4, abi_long arg5, abi_long arg6)
> +void print_freebsd_syscall(int num, abi_long arg1, abi_long arg2, abi_long 
> arg3,
> +        abi_long arg4, abi_long arg5, abi_long arg6)
>  {
> -    print_syscall(num, freebsd_scnames, ARRAY_SIZE(freebsd_scnames),
> -                  arg1, arg2, arg3, arg4, arg5, arg6);
> +
> +    print_syscall(num, freebsd_scnames, ARRAY_SIZE(freebsd_scnames), arg1, 
> arg2,
> +            arg3, arg4, arg5, arg6);

So this hunk (and some others) are doing nothing
but whitespace changes. QEMU general policy on this
sort of style fixup is:

(1) for lines of code your patch is touching because it's
making a functional change, it's recommended to fix those
lines to follow the coding style (where the metric of this is
"is scripts/checkpatch.pl happy with your patch?". checkpatch
is sometimes wrong, especially where preprocessor macros are
involved, so you need to exercise some judgement, but it will
spot all the nits like hardcoded tabs, missing braces, etc.
I noticed a lot of your patches are failing checkpatch.

(2) don't include in patches sections like this which
are *only* making whitespace or style changes: it makes
it harder to see what the patch is actually doing when
the functional changes are mixed up with whitespace changes

(3) mostly we prefer not to do "update whole file to
the coding style" patches, because they break git blame
and similar tools. However if you really want to do them
keep them in completely separate patches so those can
be reviewed as "should make no code changes".

thanks
-- PMM