Re: [PATCH 1/4] Implemement support for groups of syscalls in the xml-syscall interface.

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Sunday, November 02 2014, Gabriel Krisman Bertazi wrote:

> This implements support for groups of syscalls in the xml-syscall
> interface.
>
> It is done by maintaining a list of syscall_group_desc for each syscall
> group inside the syscalls_info structure.  Inside each
> syscall_group_desc we have a vector of pointers to the syscalls that are
> part of that group.
>
> I also experimented with storing the group info inside each syscall_desc
> element, but that wasn't very practical when dealing with syscalls that
> are part of more than one group. :)

Thanks for the patch.  It looks very good now.  I have only a few
comments, just nitpicking.

> gdb/
>
> 	* syscalls/gdb-syscalls.dtd: Include group attribute to the
> 	syscall element.
> 	* xml-syscall.c (get_syscalls_by_group): New.
> 	(get_syscall_group_names): New.
> 	(struct syscall_group_desc): New structure to store group data.
> 	(struct syscalls_info): Include field to store the group list.
> 	(sysinfo_free_syscall_group_desc): New.
> 	(free_syscalls_info): Free group list.
> 	(syscall_group_create_syscall_group_desc): New.
> 	(syscall_group_add_syscall): New.
> 	(syscall_create_syscall_desc): Add syscall to its groups.
> 	(syscall_start_syscall): Load group attribute.
> 	(syscall_group_get_group_by_name): New.
> 	(xml_list_syscalls_by_group): New.
> 	(xml_list_of_groups): New.
> 	* xml-syscall.h (get_syscalls_by_group): Export function
> 	to retrieve a list of syscalls filtered by the group name.
> 	(get_syscall_group_names): Export function to retrieve the list
> 	of syscall groups.
> ---
>  gdb/syscalls/gdb-syscalls.dtd |   3 +-
>  gdb/xml-syscall.c             | 219 +++++++++++++++++++++++++++++++++++++++++-
>  gdb/xml-syscall.h             |  12 +++
>  3 files changed, 231 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/syscalls/gdb-syscalls.dtd b/gdb/syscalls/gdb-syscalls.dtd
> index 3ad3625..b60eb74 100644
> --- a/gdb/syscalls/gdb-syscalls.dtd
> +++ b/gdb/syscalls/gdb-syscalls.dtd
> @@ -11,4 +11,5 @@
>  <!ELEMENT syscall		EMPTY>
>  <!ATTLIST syscall
>  	name			CDATA	#REQUIRED
> -	number			CDATA	#REQUIRED>
> +	number			CDATA	#REQUIRED
> +	groups			CDATA	#OPTIONAL>
> diff --git a/gdb/xml-syscall.c b/gdb/xml-syscall.c
> index 3824468..06be330 100644
> --- a/gdb/xml-syscall.c
> +++ b/gdb/xml-syscall.c
> @@ -76,6 +76,20 @@ get_syscall_names (void)
>    return NULL;
>  }
>  
> +struct syscall *
> +get_syscalls_by_group (const char *group)
> +{
> +  syscall_warn_user ();
> +  return NULL;
> +}
> +
> +const char **
> +get_syscall_group_names (void)
> +{
> +  syscall_warn_user ();
> +  return NULL;
> +}
> +
>  #else /* ! HAVE_LIBEXPAT */
>  
>  /* Variable that will hold the last known data-directory.  This is useful to
> @@ -95,12 +109,29 @@ typedef struct syscall_desc
>  } *syscall_desc_p;
>  DEF_VEC_P(syscall_desc_p);
>  
> +/* Structure of a syscall group.  */
> +typedef struct syscall_group_desc
> +{
> +  /* The group name.  */
> +
> +  char *name;
> +
> +  /* The syscalls that are part of the group.  */
> +
> +  VEC(syscall_desc_p) *syscalls;
> +} *syscall_group_desc_p;
> +DEF_VEC_P(syscall_group_desc_p);
> +
>  /* Structure that represents syscalls information.  */
>  struct syscalls_info
>  {
>    /* The syscalls.  */
>  
>    VEC(syscall_desc_p) *syscalls;
> +
> +  /* The syscall groups.  */
> +
> +  VEC(syscall_group_desc_p) *groups;
>  };
>  
>  /* Callback data for syscall information parsing.  */
> @@ -134,17 +165,32 @@ sysinfo_free_syscalls_desc (struct syscall_desc *sd)
>  }
>  
>  static void
> +sysinfo_free_syscall_group_desc (struct syscall_group_desc *sd)
> +{
> +  VEC_free (syscall_desc_p, sd->syscalls);
> +  xfree (sd->name);
> +}

I know the whole xml-syscall.c file does not obey this rule, but every
new function should have a comment describing it.

> +
> +static void
>  free_syscalls_info (void *arg)
>  {
>    struct syscalls_info *sysinfo = arg;
>    struct syscall_desc *sysdesc;
> +  struct syscall_group_desc *groupdesc;
>    int i;
>  
>    for (i = 0;
>         VEC_iterate (syscall_desc_p, sysinfo->syscalls, i, sysdesc);
>         i++)
>      sysinfo_free_syscalls_desc (sysdesc);
> +
> +  for (i = 0;
> +       VEC_iterate (syscall_group_desc_p, sysinfo->groups, i, groupdesc);
> +       i++)
> +    sysinfo_free_syscall_group_desc (groupdesc);
> +
>    VEC_free (syscall_desc_p, sysinfo->syscalls);
> +  VEC_free (syscall_group_desc_p, sysinfo->groups);
>  
>    xfree (sysinfo);
>  }
> @@ -155,15 +201,66 @@ make_cleanup_free_syscalls_info (struct syscalls_info *sysinfo)
>    return make_cleanup (free_syscalls_info, sysinfo);
>  }
>  
> +/* Create a new syscall group.  Returns a pointer to the
> +   syscall_group_desc structure that represents the new group.  */

It is annoying, but we try to write comments using the imperative
language.  Therefore, we use "Return a pointer..." instead of "Returns a
pointer...".  Anyway, this is just a very minor nit, but I myself was
corrected about it before :-).

> +
> +static struct syscall_group_desc *
> +syscall_group_create_syscall_group_desc (struct syscalls_info *sysinfo,
> +					 const char *group)
> +{
> +  struct syscall_group_desc *groupdesc = XCNEW (struct syscall_group_desc);
> +
> +  groupdesc->name = xstrdup (group);
> +
> +  VEC_safe_push (syscall_group_desc_p, sysinfo->groups, groupdesc);
> +
> +  return groupdesc;
> +}
> +
> +/* Add a syscall to a group.  If the group doesn't exist, creates
> +   it.  */
> +
> +static void
> +syscall_group_add_syscall (struct syscalls_info *sysinfo,
> +			   struct syscall_desc *syscall,
> +			   const char *group)
> +{
> +  struct syscall_group_desc *groupdesc;
> +  int i;
> +
> +  /* Search for an existing group.  */
> +  for (i = 0;
> +       VEC_iterate (syscall_group_desc_p, sysinfo->groups, i, groupdesc);
> +       i++)
> +    if (strcmp (groupdesc->name, group) == 0)
> +      break;
> +
> +  if (groupdesc == NULL)
> +    {
> +      /* No group was found with this name.  We must create a new
> +	 one.  */
> +      groupdesc = syscall_group_create_syscall_group_desc (sysinfo, group);
> +    }
> +
> +  VEC_safe_push (syscall_desc_p, groupdesc->syscalls, syscall);
> +}
> +
>  static void
>  syscall_create_syscall_desc (struct syscalls_info *sysinfo,
> -                             const char *name, int number)
> +			     const char *name, int number,
> +			     char *groups)
>  {
>    struct syscall_desc *sysdesc = XCNEW (struct syscall_desc);
> +  char *group;
>  
>    sysdesc->name = xstrdup (name);
>    sysdesc->number = number;
>  
> +  /*  Add syscall to its groups.  */
> +  if (groups != NULL)
> +    for (group = strtok (groups, ","); group; group = strtok (NULL, ","))

This should use explicit comparison, i.e., "group != NULL".

> +      syscall_group_add_syscall (sysinfo, sysdesc, group);
> +
>    VEC_safe_push (syscall_desc_p, sysinfo->syscalls, sysdesc);
>  }
>  
> @@ -179,6 +276,7 @@ syscall_start_syscall (struct gdb_xml_parser *parser,
>    /* syscall info.  */
>    char *name = NULL;
>    int number = 0;
> +  char *groups = NULL;
>  
>    len = VEC_length (gdb_xml_value_s, attributes);
>  
> @@ -188,13 +286,15 @@ syscall_start_syscall (struct gdb_xml_parser *parser,
>          name = attrs[i].value;
>        else if (strcmp (attrs[i].name, "number") == 0)
>          number = * (ULONGEST *) attrs[i].value;
> +      else if (strcmp (attrs[i].name, "groups") == 0)
> +        groups = attrs[i].value;
>        else
>          internal_error (__FILE__, __LINE__,
>                          _("Unknown attribute name '%s'."), attrs[i].name);
>      }
>  
>    gdb_assert (name);
> -  syscall_create_syscall_desc (data->sysinfo, name, number);
> +  syscall_create_syscall_desc (data->sysinfo, name, number, groups);
>  }
>  
>  
> @@ -202,6 +302,7 @@ syscall_start_syscall (struct gdb_xml_parser *parser,
>  static const struct gdb_xml_attribute syscall_attr[] = {
>    { "number", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
>    { "name", GDB_XML_AF_NONE, NULL, NULL },
> +  { "groups", GDB_XML_AF_OPTIONAL, NULL, NULL },
>    { NULL, GDB_XML_AF_NONE, NULL, NULL }
>  };
>  
> @@ -315,6 +416,32 @@ init_sysinfo (void)
>    my_gdb_datadir = xstrdup (gdb_datadir);
>  }
>  
> +/* Search for a syscall group by its name.  Returns syscall_group_desc
> +   structure for the group if found or NULL otherwise.  */
> +
> +static struct syscall_group_desc *
> +syscall_group_get_group_by_name (const struct syscalls_info *sysinfo,
> +				 const char *group)
> +{
> +  struct syscall_group_desc *groupdesc;
> +  int i;
> +
> +  if (sysinfo == NULL)
> +    return NULL;
> +
> +  if (group == NULL)
> +    return NULL;
> +
> +   /* Search for existing group.  */
> +  for (i = 0;
> +       VEC_iterate (syscall_group_desc_p, sysinfo->groups, i, groupdesc);
> +       i++)
> +    if (strcmp (groupdesc->name, group) == 0)
> +      return groupdesc;
> +
> +  return NULL;
> +}
> +
>  static int
>  xml_get_syscall_number (const struct syscalls_info *sysinfo,
>                          const char *syscall_name)
> @@ -379,6 +506,72 @@ xml_list_of_syscalls (const struct syscalls_info *sysinfo)
>    return names;
>  }
>  
> +/*  Iterate over the syscall_group_desc element to return a list of
> +    syscalls that are part of the given group, terminated by an empty
> +    element.  If the syscall group doesn't exist, returns NULL.  */
> +
> +static struct syscall *
> +xml_list_syscalls_by_group (const struct syscalls_info *sysinfo,
> +			    const char *group)
> +{
> +  struct syscall_group_desc *groupdesc;
> +  struct syscall_desc *sysdesc;
> +  struct syscall *syscalls = NULL;
> +  int nsyscalls;
> +  int i;
> +
> +  if (sysinfo == NULL)
> +    return NULL;
> +
> +  groupdesc = syscall_group_get_group_by_name (sysinfo, group);
> +  if (groupdesc == NULL)
> +    return NULL;
> +
> +  nsyscalls = VEC_length (syscall_desc_p, groupdesc->syscalls);
> +  syscalls = xmalloc ((nsyscalls + 1) * sizeof (struct syscall));
> +
> +  for (i = 0;
> +       VEC_iterate (syscall_desc_p, groupdesc->syscalls, i, sysdesc);
> +       i++)
> +    {
> +      syscalls[i].name = sysdesc->name;
> +      syscalls[i].number = sysdesc->number;
> +    }
> +
> +  /* Add final element marker.  */
> +  syscalls[i].name = NULL;
> +  syscalls[i].number = 0;
> +
> +  return syscalls;
> +}
> +
> +/* Returns the list of syscall groups.  If no syscall group is
> +   available, returns NULL.  */
> +
> +static const char **
> +xml_list_of_groups (const struct syscalls_info *sysinfo)
> +{
> +  struct syscall_group_desc *groupdesc;
> +  const char **names = NULL;
> +  int i;
> +  int ngroups;
> +
> +  if (sysinfo == NULL)
> +    return NULL;
> +
> +  ngroups = VEC_length (syscall_group_desc_p, sysinfo->groups);
> +  names = xmalloc ((ngroups + 1) * sizeof (char *));
> +
> +  for (i = 0;
> +       VEC_iterate (syscall_group_desc_p, sysinfo->groups, i, groupdesc);
> +       i++)
> +    names[i] = groupdesc->name;
> +
> +  names[i] = NULL;
> +
> +  return names;
> +}
> +
>  void
>  set_xml_syscall_file_name (const char *name)
>  {
> @@ -413,4 +606,26 @@ get_syscall_names (void)
>    return xml_list_of_syscalls (sysinfo);
>  }
>  
> +/* Returns a list of syscalls that are part of a group, terminated by an
> +   empty element.  If group doesn't exist, returns NULL.  */
> +
> +struct syscall *
> +get_syscalls_by_group (const char *group)
> +{
> +  init_sysinfo ();
> +
> +  return xml_list_syscalls_by_group (sysinfo, group);
> +}

Instead of duplicating the comment for the function here, you could say
"See comment in xml-syscall.h".

> +
> +/* Return a list of available groups.  If there are no groups available,
> +   returns NULL.  */
> +
> +const char **
> +get_syscall_group_names (void)
> +{
> +  init_sysinfo ();
> +
> +  return xml_list_of_groups (sysinfo);
> +}
> +
>  #endif /* ! HAVE_LIBEXPAT */
> diff --git a/gdb/xml-syscall.h b/gdb/xml-syscall.h
> index dff389d..573b588 100644
> --- a/gdb/xml-syscall.h
> +++ b/gdb/xml-syscall.h
> @@ -47,4 +47,16 @@ void get_syscall_by_name (const char *syscall_name, struct syscall *s);
>  
>  const char **get_syscall_names (void);
>  
> +/* Function used to retrieve the list of syscalls of a given group in
> +   the system.  Returns a list of syscalls that are element of the
> +   group, terminated by an empty element.  If group doesn't exist,
> +   returns NULL.  */
> +
> +struct syscall *get_syscalls_by_group (const char *group);

And here, you could explicitly mention that the list returned is
dinamically allocated and should be freed by the caller.

> +
> +/* Function used to retrieve the list of syscall groups in the system.
> +   Returns an array of strings terminated by a NULL element.  */
> +
> +const char **get_syscall_group_names (void);
> +
>  #endif /* XML_SYSCALL_H */
> -- 
> 1.9.3

Otherwise, the patch looks good to me (I am not the maintainer of this
portion of the code).

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/