On Wed, Jun 28, 2017 at 05:56:40AM +0300, Edgar Kaziahmedov wrote:
> From: Edgar Kaziahmedov <e...@linux.com>
>
> The first version of asinfo tool. The asinfo tool has the
> following architecture:
> architecture dispatcher->system call dispatcher->filter dispatcher
>
> As for arch. dispatcher, the asinfo has get-arch, list-arch, set-arch
> %name_of_arch options, arch_dispatcher is purposed to process them.
> Now, to find out the main principle of working, let’s consider the next
> one case:
> $ asinfo list-syscall set-filter signal
> Firstly, arch_dispatcher will return the current architecture, after
> that syscall_dispatcher will provide the list of system calls
> (struct sc_base_info) for this architecture, finally filter_dispatcher,
> working with syscallent’s database, will drop all system calls, which
> are not signal-related.
> For now, tool is able to show syscall typing its sequence number.
>
> * Makefile.am (SUBDIRS): Add tools directory.
> * configure.ac (AC_CONFIG_FILES): Add Makefile's.
> * tools/Makefile.am: New file.
> * tools/asinfo/Makefile.am: New file.
> * tools/asinfo/arch_list.h: New file.
> * tools/asinfo/dispatchers.h: New file. Prototype syscall_dispatcher
> and arch_dispatcher.
> * tools/asinfo/dispatchers.c: New file. Implement arch_dispatcher.
> Import syscallent.h files.
> * tools/asinfo/arch_interface.h: New file. Prototype methods to simplify
> work with the list of arch_descriptor. Introduce struct arch_descriptor.
> * tools/asinfo/arch_interface.c: New file. Implement them.
> * tools/asinfo/request_msgs.h: New file. Introduce main type of requests.
> Introduce structures for syscall_dispatcher.
> * tools/asinfo/asinfo.c: New file. Implement support of all arch options and
> initial support of syscall options.
> * tools/asinfo/asinfo.1: New file. Empty man.
>
> Signed-off-by: Edgar Kaziahmedov <e...@linux.com>
This and previous commit are signed with different e-mails, is this on
purpose?
> ---
> Makefile.am | 2 +-
> configure.ac | 2 +
> tools/Makefile.am | 28 ++++++
> tools/asinfo/Makefile.am | 56 ++++++++++++
> tools/asinfo/arch_interface.c | 101 ++++++++++++++++++++++
> tools/asinfo/arch_interface.h | 75 +++++++++++++++++
> tools/asinfo/arch_list.h | 4 +
> tools/asinfo/asinfo.1 | 0
> tools/asinfo/asinfo.c | 192
> ++++++++++++++++++++++++++++++++++++++++++
> tools/asinfo/dispatchers.c | 184 ++++++++++++++++++++++++++++++++++++++++
> tools/asinfo/dispatchers.h | 54 ++++++++++++
> tools/asinfo/request_msgs.h | 80 ++++++++++++++++++
> 12 files changed, 777 insertions(+), 1 deletion(-)
> create mode 100644 tools/Makefile.am
> create mode 100644 tools/asinfo/Makefile.am
> create mode 100644 tools/asinfo/arch_interface.c
> create mode 100644 tools/asinfo/arch_interface.h
> create mode 100644 tools/asinfo/arch_list.h
> create mode 100644 tools/asinfo/asinfo.1
> create mode 100644 tools/asinfo/asinfo.c
> create mode 100644 tools/asinfo/dispatchers.c
> create mode 100644 tools/asinfo/dispatchers.h
> create mode 100644 tools/asinfo/request_msgs.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 233796b6..3dc78eb5 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -35,7 +35,7 @@ endif
> if HAVE_MX32_RUNTIME
> TESTS_MX32 = tests-mx32
> endif
> -SUBDIRS = tests $(TESTS_M32) $(TESTS_MX32)
> +SUBDIRS = tests $(TESTS_M32) $(TESTS_MX32) tools
>
> bin_PROGRAMS = strace
> man_MANS = strace.1
> diff --git a/configure.ac b/configure.ac
> index bc7b637d..55fb978d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -825,6 +825,8 @@ AC_CONFIG_FILES([Makefile
> tests/Makefile
> tests-m32/Makefile
> tests-mx32/Makefile
> + tools/Makefile
> + tools/asinfo/Makefile
> strace.spec
> debian/changelog])
> AC_OUTPUT
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> new file mode 100644
> index 00000000..6044e9ae
> --- /dev/null
> +++ b/tools/Makefile.am
> @@ -0,0 +1,28 @@
> +# Automake input for strace tools.
> +#
> +# Copyright (c) 2017 Edgar A. Kaziakhmedov <e...@linux.com>
> +# All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +# 1. Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +# 2. Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +# 3. The name of the author may not be used to endorse or promote products
> +# derived from this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> +# IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> +# OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> +# IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> +# INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> +# NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> +# THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +SUBDIRS = asinfo
> diff --git a/tools/asinfo/Makefile.am b/tools/asinfo/Makefile.am
> new file mode 100644
> index 00000000..195dfe84
> --- /dev/null
> +++ b/tools/asinfo/Makefile.am
> @@ -0,0 +1,56 @@
> +# Automake input for strace.
"for strace"?
> +#
> +# Copyright (c) 2017 Edgar Kaziakhmedov <e...@linux.com>
> +# All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +# 1. Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +# 2. Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +# 3. The name of the author may not be used to endorse or promote products
> +# derived from this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> +# IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> +# OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> +# IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> +# INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> +# NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> +# THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +bin_PROGRAMS = asinfo
> +man_MANS = asinfo.1
> +
> +OS = linux
> +ARCH = @arch@
> +
> +AM_CFLAGS = $(WARN_CFLAGS)
> +AM_CPPFLAGS = -I$(builddir) \
> + -I$(top_builddir)/$(OS) \
> + -I$(top_srcdir)/$(OS) \
> + -I$(top_builddir) \
> + -I$(top_srcdir)
> +asinfo_CPPFLAGS = $(AM_CPPFLAGS)
> +asinfo_CFLAGS = $(AM_CFLAGS)
> +
> +asinfo_SOURCES = \
Tabs after space.
> + arch_interface.c \
> + arch_interface.h \
> + arch_list.h \
> + asinfo.c \
> + dispatchers.c \
> + dispatchers.h \
> + ../../error_prints.c \
> + ../../error_prints.h \
> + ../../macros.h \
> + requests_msgs.h \
> + ../../xmalloc.c \
> + ../../xmalloc.h \
> + #end of asinfo_SOURCES
> diff --git a/tools/asinfo/arch_interface.c b/tools/asinfo/arch_interface.c
> new file mode 100644
> index 00000000..3c11beae
> --- /dev/null
> +++ b/tools/asinfo/arch_interface.c
> @@ -0,0 +1,101 @@
> +/*
File description will not harm here.
> + * Copyright (c) 2017 Edgar A. Kaziakhmedov <e...@linux.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + * derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "arch_interface.h"
> +#include "xmalloc.h"
> +
> +struct arch_service *arch_list_create(unsigned size)
Shouldn't the argument be called "capacity"?
Function return type should be on a separate line.
> +{
> + struct arch_service *as = (struct arch_service *) \
Newline escaping is not needed here.
> + xcalloc(sizeof(*as), 1);
It's better to format line continuation like this:
struct arch_service *as =
(struct arch_service *) xcalloc(sizeof(*as), 1);
Moreover, this cast is not needed.
> + if (size) {
> + as->arch_list = (struct arch_descriptor **) \
> + xcalloc(sizeof(*(as->arch_list)), size);
The same regarding line continuation formatting and cast.
> + }
Compound statement braces can be omitted in this case.
> + as->capacity = size;
> + as->next_free = 0;
> + return as;
> +}
> +
> +int arch_list_push(struct arch_service *m, struct arch_descriptor *element)
Function return type should be on a separate line.
> +{
> + if (m->next_free >= m->capacity) {
> + return -1;
> + }
Compound statement braces can be omitted in this case.
> + m->arch_list[m->next_free] = element;
> + (m->next_free)++;
Parentheses are not needed here.
> + return 0;
> +}
> +
> +struct arch_descriptor *arch_list_get(struct arch_service *m, unsigned index)
Function return type should be on a separate line.
> +{
> + if (index >= m->next_free) {
> + return NULL;
> + }
Compound statement braces can be omitted in this case.
> + return m->arch_list[index];
> +}
> +
> +unsigned arch_list_size(struct arch_service *m)
Function return type should be on a separate line.
> +{
> + return m->next_free;
> +}
> +
> +void arch_list_free(struct arch_service *m)
Function return type should be on a separate line.
> +{
> + free(m->arch_list);
> + free(m);
> +}
> +
> +const char* arch_list_name(struct arch_service *m, unsigned index)
Function return type should be on a separate line.
> +{
Why there's no next_free check? I'd implement it via arch_list_get with
a check for NULL afterwards.
> + return m->arch_list[index]->arch_name;
> +}
> +
> +int arch_list_snum(struct arch_service *m, unsigned index)
Function return type should be on a separate line.
> +{
The same regarding next_free check.
> + return m->arch_list[index]->max_scn;
> +}
> +
> +void arch_list_print(struct arch_service *m, const char *delim)
Function return type should be on a separate line.
> +{
> + unsigned i = 0;
> + unsigned max_len = 0;
No newline after declarations.
> + for (i = 0; i < arch_list_size(m); i++) {
> + unsigned temp = strlen(m->arch_list[i]->arch_name);
Why direct access if the accessor function was defined?
Btw, since arch_name values are static strings, one can calculate their
length in compile-time in store in a separate field. So much for
premature optimisation.
> + if (temp > max_len) {
> + max_len = temp;
> + }
Braces can be omitted here.
> + }
> + for (i = 0; i < arch_list_size(m); i++) {
> + printf("[%u] %*s %s %d\n", i + 1, max_len, arch_list_name(m, i),
This output will be unaligned once arch_list_size will be grown up to 11.
> + delim, arch_list_snum(m, i));
> + }
Braces can be omitted here.
> +}
> diff --git a/tools/asinfo/arch_interface.h b/tools/asinfo/arch_interface.h
> new file mode 100644
> index 00000000..5e9a8256
> --- /dev/null
> +++ b/tools/asinfo/arch_interface.h
> @@ -0,0 +1,75 @@
> +/*
File description will not harm here.
> + * Copyright (c) 2017 Edgar A. Kaziakhmedov <e...@linux.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + * derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
Missing newline.
> +#ifndef ASINFO_ARCH_INTERFACE
> +#define ASINFO_ARCH_INTERFACE
> +
> +#include "sysent.h"
> +
> +enum arch_types {
"enum arch_type" would be better.
> + AARCH64,
> + ARM,
> + x86_64,
> + x32
> +};
> +
> +struct arch_descriptor {
> + enum arch_types arch_num;
> + const char *arch_name;
> + int max_scn;
> + struct_sysent *list_of_syscall;
> +};
> +
> +/* To provide push-back interface with arch_list */
> +struct arch_service {
> + struct arch_descriptor **arch_list;
> + unsigned capacity;
> + unsigned next_free;
> +};
> +
> +#define ARCH_LIST_DEFINE(name) \
> + struct arch_service *(name)
> +
> +/* Push-back interface is purposed to simplify interaction with
> + array of struct arch_descriptor's pointers */
> +
> +struct arch_service *arch_list_create(unsigned size);
> +
> +int arch_list_push(struct arch_service *m, struct arch_descriptor *element);
> +
> +struct arch_descriptor *arch_list_get(struct arch_service *m, unsigned
> index);
Oversized line.
> +
> +unsigned arch_list_size(struct arch_service *m);
> +
> +void arch_list_free(struct arch_service *m);
> +
> +const char* arch_list_name(struct arch_service *m, unsigned index);
> +
> +int arch_list_snum(struct arch_service *m, unsigned index);
> +
> +void arch_list_print(struct arch_service *m, const char *delim);
> +
> +#endif /* !ASINFO_ARCH_INTERFACE */
> diff --git a/tools/asinfo/arch_list.h b/tools/asinfo/arch_list.h
> new file mode 100644
> index 00000000..4a7accf7
> --- /dev/null
> +++ b/tools/asinfo/arch_list.h
> @@ -0,0 +1,4 @@
> +[0] = { AARCH64, "aarch64", ARRAY_SIZE(aarch64_sysent0),
> aarch64_sysent0 },
> +[1] = { ARM, "arm", ARRAY_SIZE(arm_sysent0),
> arm_sysent0 },
> +[2] = { x86_64, "x86_64", ARRAY_SIZE(x86_64_sysent0),
> x86_64_sysent0 },
> +[3] = { x32, "x32", ARRAY_SIZE(x32_sysent0),
> x32_sysent0 },
You can define utility macro which allows avoiding most of the
duplication (assuming that you rename arch names in enum to ARCH_*):
#define ARCH_DESC_DEFINE(arch) \
[ARCH_##arch] = { \
.arch_num = ARCH_##arch, \
.arch_name = #arch, \
.max_scn = ARRAY_SIZE(arch##_sysent0), \
.list_of_syscall = arch##_sysent0, \
} \
Moreover, as of now, I do not see why storing this list in a separate
file is needed, it is included currently only in a single place and it's
not going to be extremely big and frequently-updated.
> diff --git a/tools/asinfo/asinfo.1 b/tools/asinfo/asinfo.1
> new file mode 100644
> index 00000000..e69de29b
> diff --git a/tools/asinfo/asinfo.c b/tools/asinfo/asinfo.c
> new file mode 100644
> index 00000000..020ef7c8
> --- /dev/null
> +++ b/tools/asinfo/asinfo.c
> @@ -0,0 +1,192 @@
> +/*
File description will not harm here.
> + * Copyright (c) 2017 Edgar A. Kaziakhmedov <e...@linux.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + * derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <stdarg.h>
> +#include <stdbool.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <limits.h>
It's better to have this list sorted.
> +
> +#include "arch_interface.h"
> +#include "dispatchers.h"
> +#include "error_prints.h"
> +#include "macros.h"
> +#include "request_msgs.h"
> +#include "xmalloc.h"
> +
> +const char *progname;
> +static const char *def_delim = ":";
> +
> +void ATTRIBUTE_NOINLINE
Maybe ATTRIBUTE_NORETURN too? Also, I do not really grasp the need of
making this function strictly non-inline. In both cases where this
attribute is used in strace.c, the usage is this attribute is justified.
> +die(void)
> +{
> + exit(1);
> +}
> +
> +/* To calculate sequence number of set bit */
https://graphics.stanford.edu/~seander/bithacks.html#IntegerLogDeBruijn
https://graphics.stanford.edu/~seander/bithacks.html#ZerosOnRightMultLookup
> +static inline unsigned
> +fsbit_num(unsigned flag)
> +{
> + unsigned i = 0;
> + while ((flag >>= 1) != 0) i++;
> + return i;
> +}
> +
> +static unsigned
> +option_to_request(char *option)
> +{
> + const struct opt_to_req requests[] = {
static?
> + [ 0] = { AD_REQ_SET_ARCH, "set-arch" },
> + [ 1] = { AD_REQ_GET_ARCH, "get-arch" },
> + [ 2] = { AD_REQ_LIST_ARCH, "list-arch" },
> + [ 3] = { SD_REQ_GET_NAME, "get-name" },
> + //[ 4] = { SD_REQ_GET_NUMBER, "get-num" },
> + //[ 5] = { SD_REQ_GET_NARGS, "get-nargs" },
> + //[ 6] = { SD_REQ_GET_PROTO, "get-proto" },
> + //[ 7] = { SD_REQ_GET_KERNEL, "get-kernel" },
> + //[ 8] = { SD_REQ_GET_LIST, "list-syscall" },
> + //[ 9] = { FD_SET_BASE_FILTER, "set-filter" },
> + //[10] = { FD_SET_ADV_FILTER, "set-adv-filter" }
Why do you need array indices in this definition?
> + };
> + unsigned i;
> +
> + for (i = 0; i < ARRAY_SIZE(requests); i++) {
> + if (strcasecmp(option, requests[i].option) == 0)
> + return requests[i].req_t;
> + }
> + return 0;
> +}
> +
> +static unsigned ATTRIBUTE_NOINLINE
Why noinline?
> +form_req(int argc, char *argv[], char *args[])
> +{
> + progname = argv[0] ? argv[0] : "asinfo";
I would probably like to check for argc here too, see zerorgc test.
> + int i;
> + unsigned ret_val = 0;
> + unsigned temp_ret = 0;
> + unsigned bit_pos = 0;
> +
> + for (i = 1; i < argc; i++) {
> + if ((temp_ret = option_to_request(argv[i])) != 0) {
> + if ((ret_val & temp_ret) ||
> + (!(temp_ret & (SD_REQ_GET_LIST +
It's better to use bitwise or and not arithmetic plus here.
> + AD_REQ_LIST_ARCH +
> + AD_REQ_GET_ARCH)) &&
> + (i + 1 >= argc))) {
> + error_msg_and_help("option '%s' requires "
> + "argument", argv[i]);
$ ./asinfo get-name 0 get-name 1
./asinfo: option 'get-name' requires argument
Try './asinfo -h' for more information.
$
> + }
> + ret_val |= temp_ret;
> + bit_pos = fsbit_num(temp_ret);
> + if (!(temp_ret & (SD_REQ_GET_LIST + AD_REQ_LIST_ARCH +
> + AD_REQ_GET_ARCH))) {
Since this check is here for the second time, I think it can be
refactored and the related flag can be a part of some request description
struct.
> + args[bit_pos] = argv[i + 1];
> + i += 1;
> + }
> + } else {
> + error_msg_and_help("unrecognized option '%s'", argv[i]);
> + }
> + }
> + return ret_val;
> +}
> +
> +static int
> +check_req(unsigned *request, char *args[])
> +{
> + int a_flag = *request & (AD_REQ_MASK ^ AD_REQ_SET_ARCH);
> + int s_flag = *request & SD_REQ_MASK;
> + int dump = 1;
> + /* Check arch flags */
> + if (!(*request & AD_REQ_MASK)) {
> + *request |= AD_REQ_GET_ARCH;
> + dump = 0;
> + }
> + if (*request & AD_REQ_SET_ARCH) {
> + dump = 0;
> + }
> + /* Check for ambiguity */
> + if (!((*request & AD_REQ_MASK) == AD_REQ_SET_ARCH ||
> + (*request & AD_REQ_MASK) == AD_REQ_GET_ARCH ||
> + (*request & AD_REQ_MASK) == AD_REQ_LIST_ARCH)) {
> + return -1;
> + }
> + if (a_flag != 0 && s_flag != 0) {
> + return -1;
> + }
> + /* Check syscall flags, ambiguity */
> + if (!(*request & SD_REQ_MASK)) {
> + *request |= SD_REQ_GET_LIST;
> + }
> + if (!((*request & SD_REQ_MASK) == SD_REQ_GET_NAME ||
> + (*request & SD_REQ_MASK) == SD_REQ_GET_NUMBER ||
> + (*request & SD_REQ_MASK) == SD_REQ_GET_LIST)) {
> + return -1;
> + }
> + return dump;
> +}
This whole function is very cryptic.
> +
> +int
> +main(int argc, char *argv[])
> +{
> + char **input_args = (char**)xcalloc(sizeof(char*),
Missing space before xcalloc. Also this cast is not needed.
Missing spaces after "char".
> + sizeof(unsigned) * CHAR_BIT);
Line continuation should be aligned to opening parenthesis of xcalloc call.
> + unsigned requests;
> + int dump;
> +
> + if ((requests = form_req(argc, argv, input_args)) == 0)
> + error_msg_and_help("must have OPTIONS");
> + if ((dump = check_req(&requests, input_args)) == -1)
> + error_msg_and_help("ambiguous options");
> +
> + /* arch_dispatcher work */
> + ARCH_LIST_DEFINE(arch_list);
> + arch_list = arch_dispatcher(requests,
> + input_args[fsbit_num(AD_REQ_SET_ARCH)]);
> + if (dump) {
> + arch_list_print(arch_list, def_delim);
> + goto done;
> + }
> + if (arch_list_size(arch_list) == 0) {
> + error_msg_and_help("unrecognized architecture '%s'",
> + input_args[fsbit_num(AD_REQ_SET_ARCH)]);
> + }
Braces can be omitted here.
> + /* syscall_dispatcher work */
> + SYSCALL_LIST_DEFINE(syscall_list);
> + syscall_list = syscall_dispatcher(arch_list, requests,
> + input_args[fsbit_num(requests & SD_REQ_MASK)]);
> + if (syscall_list[0].sys_name != NULL) {
> + printf("%s\n", syscall_list[0].sys_name);
> + } else {
> + error_msg_and_help("unrecognized syscall number '%s'",
> + input_args[fsbit_num(requests & SD_REQ_MASK)]);
> + }
I do not really understand why exactly this logic is realized.
For example, how can one lookup syscall statistics ("dump") for a
specific architecture?
> +done:
> + arch_list_free(arch_list);
> + free(input_args);
There's no need to free allocated memory right before the exit.
> + return 0;
> +}
> diff --git a/tools/asinfo/dispatchers.c b/tools/asinfo/dispatchers.c
> new file mode 100644
> index 00000000..95ca4923
> --- /dev/null
> +++ b/tools/asinfo/dispatchers.c
> @@ -0,0 +1,184 @@
> +/*
File description will not harm here.
> + * Copyright (c) 2017 Edgar A. Kaziakhmedov <e...@linux.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + * derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/utsname.h>
It's better to have this list sorted.
> +
> +#include "arch_interface.h"
> +#include "dispatchers.h"
> +#include "macros.h"
> +#include "request_msgs.h"
> +#include "sysent.h"
> +#include "xmalloc.h"
> +
> +/* Define these shorthand notations to simplify the syscallent files. */
> +/* These flags could be useful to print group of syscalls */
> +#define TD TRACE_DESC
> +#define TF TRACE_FILE
> +#define TI TRACE_IPC
> +#define TN TRACE_NETWORK
> +#define TP TRACE_PROCESS
> +#define TS TRACE_SIGNAL
> +#define TM TRACE_MEMORY
> +#define TST TRACE_STAT
> +#define TLST TRACE_LSTAT
> +#define TFST TRACE_FSTAT
> +#define TSTA TRACE_STAT_LIKE
> +#define TSF TRACE_STATFS
> +#define TFSF TRACE_FSTATFS
> +#define TSFA TRACE_STATFS_LIKE
> +#define NF SYSCALL_NEVER_FAILS
> +#define MA MAX_ARGS
> +#define SI STACKTRACE_INVALIDATE_CACHE
> +#define SE STACKTRACE_CAPTURE_ON_ENTER
> +#define CST COMPAT_SYSCALL_TYPES
> +
> +/*
> + * For the current functionality there is no need
> + * to use sen and (*sys_func)() fields in sysent struct
> + */
> +#define SEN(syscall_name) 0, NULL
> +
> +struct_sysent aarch64_sysent0[] = {
> +#include "aarch64/syscallent.h"
> +};
> +
> +struct_sysent arm_sysent0[] = {
> +#include "arm/syscallent.h"
> +};
> +
> +struct_sysent x32_sysent0[] = {
> +#include "x32/syscallent.h"
> +};
> +
> +struct_sysent x86_64_sysent0[] = {
> +#include "x86_64/syscallent.h"
> +};
> +
> +#undef SEN
> +#undef TD
> +#undef TF
> +#undef TI
> +#undef TN
> +#undef TP
> +#undef TS
> +#undef TM
> +#undef TST
> +#undef TLST
> +#undef TFST
> +#undef TSTA
> +#undef TSF
> +#undef TFSF
> +#undef TSFA
> +#undef NF
> +#undef MA
> +#undef SI
> +#undef SE
> +#undef CST
> +
> +struct arch_descriptor architectures[] = {
> +#include "arch_list.h"
> +};
> +
> +struct sc_base_info *
> +syscall_dispatcher(struct arch_service *arch, int request_type,
> + const char *data)
> +{
> + SYSCALL_LIST_DEFINE(syscall_list);
> +
> + if (request_type & SD_REQ_GET_NAME) {
> + long int nsys;
> + char *endptr;
> + nsys = strtol(data, &endptr, 10);
Why base 10?
> + if (*endptr != '\0' || nsys < 0 ||
> + nsys >= arch->arch_list[0]->max_scn) {
> + goto fail;
> + }
$ ./asinfo get-name ""
read
$
> + SYSCALL_LIST_ALLOC(syscall_list, 1);
> + syscall_list[0].sys_name =
> + arch->arch_list[0]->list_of_syscall[nsys].sys_name;
> + goto done;
> + }
> +
> +fail:
> + SYSCALL_LIST_ALLOC(syscall_list, 0);
> +done:
> + return syscall_list;
> +}
Why creating a list and populating only a single entry?
> +
> +struct arch_service *
> +arch_dispatcher(unsigned request_type, const char* arch)
> +{
> + struct utsname info_uname;
> + const char *loc_arch;
> + unsigned i;
> + ARCH_LIST_DEFINE(arch_list);
> + if ((!arch) && (request_type & AD_REQ_SET_ARCH)) {
> + goto fail;
> + }
Braces can be omitted here.
> +
> + if (request_type & AD_REQ_GET_ARCH) {
> + uname(&info_uname);
> + for (i = 0; i < ARRAY_SIZE(architectures); i++) {
> + loc_arch = architectures[i].arch_name;
> + if (strcasestr(info_uname.machine, loc_arch) != NULL) {
I really wonder how this will behave in case of mips or powerpc.
> + arch_list = arch_list_create(1);
> + arch_list_push(arch_list, &(architectures[i]));
> + goto done;
> + }
> + }
> + }
> +
> + if (request_type & AD_REQ_SET_ARCH) {
> + for (i = 0; i < ARRAY_SIZE(architectures); i++) {
> + loc_arch = architectures[i].arch_name;
> + if (strcasestr(arch, loc_arch) != NULL) {
> + arch_list = arch_list_create(1);
> + arch_list_push(arch_list, &(architectures[i]));
> + goto done;
> + }
> + }
This loop is almost identical to the previous one, it should probably be
refactored in a separate function.
> + }
> +
> + if (request_type & AD_REQ_LIST_ARCH) {
> + arch_list = arch_list_create(ARRAY_SIZE(architectures));
> + for (i = 0; i < ARRAY_SIZE(architectures); i++) {
> + arch_list_push(arch_list, &(architectures[i]));
> + }
> + goto done;
> + }
> +
> +fail:
> + arch_list = arch_list_create(0);
> +done:
> + return arch_list;
> +}
> diff --git a/tools/asinfo/dispatchers.h b/tools/asinfo/dispatchers.h
> new file mode 100644
> index 00000000..a60f1595
> --- /dev/null
> +++ b/tools/asinfo/dispatchers.h
> @@ -0,0 +1,54 @@
> +/*
File description will not harm here.
> + * Copyright (c) 2017 Edgar A. Kaziakhmedov <e...@linux.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + * derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef STRACE_ASINFO_COM_SCALLENT
> +#define STRACE_ASINFO_COM_SCALLENT
> +
> +#include "arch_interface.h"
> +#include "request_msgs.h"
> +#include "sysent.h"
> +
> +#define SYSCALL_LIST_DEFINE(name) \
> + struct sc_base_info *(name)
> +
> +#define SYSCALL_LIST_ALLOC(name, size) \
> + (name) = (struct sc_base_info*) xcalloc(sizeof(*(name)), (size + 1))
The cast is not needed here. Missing space after sc_base_info.
> +
> +/* The main interaction with low-level structures, such as for now,
> + * struct_sysent, is happening here, the remaining processing should
> + * be done on the other levels.
> + */
> +struct sc_base_info *syscall_dispatcher(struct arch_service *arch,
> + int request_type,
> + const char *arg);
> +
> +/* The function is purposed to interact with uname syscall
> + */
> +struct arch_service *arch_dispatcher(unsigned request_type,
> + const char* arch);
> +
> +#endif /* !STRACE_ASINFO_COM_SCALLENT */
> diff --git a/tools/asinfo/request_msgs.h b/tools/asinfo/request_msgs.h
> new file mode 100644
> index 00000000..575e2735
> --- /dev/null
> +++ b/tools/asinfo/request_msgs.h
> @@ -0,0 +1,80 @@
> +/*
File description will not harm here.
> + * Copyright (c) 2017 Edgar A. Kaziakhmedov <e...@linux.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + * derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef STRACE_ASINFO_REQ_MSGS
> +#define STRACE_ASINFO_REQ_MSGS
> +
> +/* Request types for syscall_dispatcher,
> + * arch_dispatcher, and filter_dispatcher,
> + * which, in turn, could be combined
> + */
> +enum request_types {
> + SD_REQ_GET_NAME = 0x1<<0,
> + SD_REQ_GET_NUMBER = 0x1<<1,
> + SD_REQ_GET_NARGS = 0x1<<2,
> + SD_REQ_GET_PROTO = 0x1<<3,
> + SD_REQ_GET_KERNEL = 0x1<<4,
> + SD_REQ_GET_LIST = 0x1<<5,
> + /* Reserved */
> + AD_REQ_SET_ARCH = 0x1<<16,
> + AD_REQ_GET_ARCH = 0x1<<17,
> + AD_REQ_LIST_ARCH = 0x1<<18,
> + /* Reserved */
> + FD_SET_BASE_FILTER = 0x1<<24,
> + FD_SET_ADV_FILTER = 0x1<<25
What for is FD, anyway?
> +};
> +
> +#define SD_REQ_MASK (0x0000FFFFU)
> +#define AD_REQ_MASK (0x00FF0000U)
> +#define FD_REQ_MASK (0xFF000000U)
I'd have simple set of enums which calculate all this stuff for me, like
this:
enum syscall_req_bit {
SD_REQ_GET_NAME_BIT,
SD_REQ_GET_NUMBER_BIT,
SD_REQ_GET_NARGS_BIT,
SD_REQ_GET_PROTO_BIT,
SD_REQ_GET_KERNEL_BIT,
SD_REQ_GET_LIST_BIT,
SYSCALL_REQ_BIT_LAST
};
enum arch_req_bit {
AD_REQ_SET_ARCH_BIT = SYSCALL_REQ_BIT_LAST,
AD_REQ_GET_ARCH_BIT,
AD_REQ_LIST_ARCH_BIT,
ARCH_REQ_BIT_LAST
};
enum fd_req_bit {
FD_SET_BASE_FILTER_BIT = ARCH_REQ_BIT_LAST,
FD_SET_ADV_FILTER,
FD_REQ_BIT_LAST
};
#define ENUM_FLAG(name) name = name##_BIT
enum request_type {
ENUM_FLAG(SD_REQ_GET_NAME),
ENUM_FLAG(SD_REQ_GET_NUMBER),
ENUM_FLAG(SD_REQ_GET_NARGS),
ENUM_FLAG(SD_REQ_GET_PROTO),
ENUM_FLAG(SD_REQ_GET_KERNEL),
ENUM_FLAG(SD_REQ_GET_LIST),
ENUM_FLAG(AD_REQ_SET_ARCH),
ENUM_FLAG(AD_REQ_GET_ARCH),
ENUM_FLAG(AD_REQ_LIST_ARCH),
ENUM_FLAG(FD_REQ_SET_BASE_FILTER),
ENUM_FLAG(FD_REQ_SET_ADV_FILTER),
};
#undef ENUM_FLAG
#define BITMASK(hi, lo) (1 << (hi) - 1 << (lo))
#define REQ_LAST_BIT FD_REQ_BIT_LAST /* Can be used in input_args
calloc*/
#define SD_REQ_MASK BITMASK(SYSCALL_REQ_BIT_LAST, 0)
#define AD_REQ_MASK BITMASK(ARCH_REQ_BIT_LAST, SYSCALL_REQ_BIT_LAST)
#define FD_REQ_MASK BITMASK(FD_REQ_BIT_LAST, ARCH_REQ_BIT_LAST)
#undef BITMASK
This would also allow avoiding fsbit_num calls with constant operands.
> +
> +/* To convert options to req_types */
> +struct opt_to_req {
> + enum request_types req_t;
> + const char *option;
> +};
> +
> +/* SD_REQ_GET_NAME, SD_REQ_GET_NUMBER */
> +struct sc_base_info {
> + int sys_number;
> + const char *sys_name;
> + int sys_flags;
> +};
> +
> +/* SD_REQ_GET_NARGS */
> +struct sc_nargs {
> + struct sc_base_info sys_bi;
> + int nargs;
> +};
> +
> +/* FD_SET_BASE_FILTER */
> +struct sc_filter {
> + struct sc_base_info sys_bi;
> + int filter_options;
> +};
> +
> +#endif /* !STRACE_ASINFO_REQ_MSGS */
> --
> 2.11.0
$ ./asinfo -h
./asinfo: unrecognized option '-h'
Try './asinfo -h' for more information.
$
Overall, the whole thing now looks over-engineered with no significant
reason for that. Maybe future commits which implement more functionality
will solve that.
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
