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

[Peter Maydell]

On 5 June 2014 18:27, Sean Bruno <address@hidden> wrote:
> From: Stacey Son <address@hidden>
>
> 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.

> +{ 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 },

This sort of change to the set of supported syscalls for strace looks
like it should
be in a separate patch to the "move files" patch.

> -static void
> -print_syscall(int num, const struct syscallname *scnames, unsigned int 
> nscnames,
> -              abi_long arg1, abi_long arg2, abi_long arg3,
> -              abi_long arg4, abi_long arg5, abi_long arg6)
> +static void print_syscall(int num, const struct syscallname *scnames,
> +        unsigned int nscnames, abi_long arg1, abi_long arg2, abi_long arg3,
> +        abi_long arg4, abi_long arg5, abi_long arg6)
>  {
>      unsigned int i;
>      const char *format="%s(" TARGET_ABI_FMT_ld "," TARGET_ABI_FMT_ld ","
> @@ -102,36 +164,37 @@ print_syscall(int num, const struct syscallname 
> *scnames, unsigned int nscnames,
>
>      gemu_log("%d ", getpid() );
>
> -    for (i = 0; i < nscnames; i++)
> +    for (i = 0; i < nscnames; i++) {
>          if (scnames[i].nr == num) {
>              if (scnames[i].call != NULL) {
>                  scnames[i].call(&scnames[i], arg1, arg2, arg3, arg4, arg5,
> -                                arg6);
> +                        arg6);
>              } else {
>                  /* XXX: this format system is broken because it uses
>                     host types and host pointers for strings */
> -                if (scnames[i].format != NULL)
> +                if (scnames[i].format != NULL) {
>                      format = scnames[i].format;
> -                gemu_log(format, scnames[i].name, arg1, arg2, arg3, arg4,
> -                         arg5, arg6);
> +                }
> +                gemu_log(format, scnames[i].name, arg1, arg2, arg3, arg4, 
> arg5,
> +                        arg6);
>              }
>              return;
>          }
> +    }

A lot of this (and below) is just coding style fixes. Please make
sure you keep those entirely in their own patches, not mixed
in with functionality changes -- it's much easier to review
that way.

thanks
-- PMM