This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/4] Add support to catch groups of syscalls.
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Gabriel Krisman Bertazi <gabriel at krisman dot be>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 14 Nov 2014 17:55:12 -0500
- Subject: Re: [PATCH 2/4] Add support to catch groups of syscalls.
- Authentication-results: sourceware.org; auth=none
- References: <1414956944-8856-1-git-send-email-gabriel at krisman dot be> <1414956944-8856-3-git-send-email-gabriel at krisman dot be>
On Sunday, November 02 2014, Gabriel Krisman Bertazi wrote:
> This implements the catchpoint side. While parsing 'catch syscall'
> arguments, we verify if the argument is a syscall group and expand it to
> a list of syscalls that are part of that group.
Nice, looks good :-). A few comments.
> gdb/
>
> * breakpoint.c (catch_syscall_split_args): Verify if argument
> is a syscall group and expand it to a list of syscalls when
> creating catchpoints.
> (catch_syscall_completer): Add word completion for system call
> groups.
> ---
> gdb/breakpoint.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 95 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index cab6c56..8520361 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -12111,10 +12111,40 @@ catch_syscall_split_args (char *arg)
> cur_name[i] = '\0';
> arg += i;
>
> - /* Check if the user provided a syscall name or a number. */
> + /* Check if the user provided a syscall name, group, or a
> + number. */
> syscall_number = (int) strtol (cur_name, &endptr, 0);
> if (*endptr == '\0')
> - get_syscall_by_number (syscall_number, &s);
> + {
> + get_syscall_by_number (syscall_number, &s);
> + VEC_safe_push (int, result, s.number);
> + }
> + else if ((cur_name[0] == 'g' && cur_name[1] == ':')
> + || strstr (cur_name, "group:") == cur_name)
Here you compare chars and use strstr, and then below you use two calls
to strncmp. I prefer if you use strncmp here as well, because it would
be simpler.
> + {
> + /* We have a syscall group. Let's expand it into a syscall
> + list before inserting. */
> + struct syscall *syscall_list;
> + const char *group_name;
> +
> + /* Skip over "g:" and "group:" prefix strings. */
> + group_name = strchr (cur_name, ':') + 1;
> +
> + syscall_list = get_syscalls_by_group (group_name);
> +
> + if (syscall_list == NULL)
> + error (_("Unknown syscall group '%s'."), group_name);
> +
> + for (i = 0; syscall_list[i].name; i++)
Explicit comparison:
syscall_list[i].name != NULL
> + {
> + /* Insert each syscall that are part of the group. No
> + need to check if it is valid. */
> +
> + VEC_safe_push (int, result, syscall_list[i].number);
I prefer without the extra newline separating the comment and the code.
Likewise for everywhere else you did this ;-).
> + }
> +
> + xfree (syscall_list);
> + }
> else
> {
> /* We have a name. Let's check if it's valid and convert it
> @@ -12126,10 +12156,11 @@ catch_syscall_split_args (char *arg)
> because GDB cannot do anything useful if there's no
> syscall number to be caught. */
> error (_("Unknown syscall name '%s'."), cur_name);
> - }
>
> - /* Ok, it's valid. */
> - VEC_safe_push (int, result, s.number);
> + /* Ok, it's valid. */
> +
> + VEC_safe_push (int, result, s.number);
> + }
> }
>
> discard_cleanups (cleanup);
> @@ -15417,11 +15448,66 @@ static VEC (char_ptr) *
> catch_syscall_completer (struct cmd_list_element *cmd,
> const char *text, const char *word)
> {
> - const char **list = get_syscall_names ();
> - VEC (char_ptr) *retlist
> - = (list == NULL) ? NULL : complete_on_enum (list, word, word);
> + struct cleanup *cleanups;
> + VEC (char_ptr) *group_retlist = NULL;
> + VEC (char_ptr) *syscall_retlist = NULL;
> + VEC (char_ptr) *retlist = NULL;
> + const char **group_list = NULL;
> + const char **syscall_list = NULL;
> + const char *prefix;
> + int i;
> +
> + /* Completion considers ':' to be a word separator, so we use this to
> + verify whether the previous word was a group prefix. If so, we
> + build the completion list using group names only. */
> + for (prefix = word; prefix != text && *(prefix-1) != ' '; prefix--)
You can use prefix[-1] instead of *(prefix-1) here. But if you decide
to the latter, then you should write *(prefix - 1).
> + ;
> +
> + if (strncmp (prefix, "g:", strlen ("g:")) == 0
> + || strncmp (prefix, "group:", strlen ("group:")) == 0)
> + {
> + /* Perform completion inside 'group:' namespace only. */
> +
> + group_list = get_syscall_group_names ();
> + cleanups = make_cleanup (xfree, group_list);
> + retlist = (group_list == NULL) ?
> + NULL : complete_on_enum (group_list, word, word);
> + }
> + else
> + {
> + /* Complete with both, syscall names and groups. */
> +
> + syscall_list = get_syscall_names ();
> + group_list = get_syscall_group_names ();
> + cleanups = make_cleanup (xfree, group_list);
> +
> + /* Append "group:" prefix to syscall groups. */
> + for (i = 0; group_list[i]; i++)
Explicit comparison.
> + {
> + const char *group = group_list[i];
> + size_t str_length = (sizeof (char)
> + *(strlen (group) + strlen ("group:") + 1));
^^^
Missing space between operator and parenthesis.
> + char *prefixed_group = xmalloc (str_length);
> +
> + xsnprintf (prefixed_group, str_length, "group:%s", group);
You can use xstrprintf here, and drop the xmalloc dance.
> + group_list[i] = prefixed_group;
> +
> + make_cleanup (xfree, prefixed_group);
> + }
> +
> + syscall_retlist = (syscall_list == NULL) ?
> + NULL : complete_on_enum (syscall_list, word, word);
> + group_retlist = (group_list == NULL) ?
> + NULL : complete_on_enum (group_list, word, word);
> +
> + retlist = VEC_merge (char_ptr, syscall_retlist, group_retlist);
> + }
> +
> + VEC_free (char_ptr, syscall_retlist);
> + VEC_free (char_ptr, group_retlist);
> + xfree (syscall_list);
> + do_cleanups (cleanups);
>
> - xfree (list);
> return retlist;
> }
>
> --
> 1.9.3
Otherwise, the patch looks good to me (I am not the maintainer of this
part of the code).
--
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/