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

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

On Tuesday, October 07 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.

I think creating the syscall_desc list is the best approach in this case
indeed.  I have other comments about the code, while I am at it.

> gdb/
>
> 	* syscalls/gdb-syscalls.dtd: Include group attribute to the
> 	syscall element.
> 	* xml-syscall.c (get_syscall_names_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_of_syscalls): Add filter by group name.
> 	(xml_list_of_groups): New.
> 	(get_syscall_names): Call xml_list_of_syscalls with a NULL group
> 	filter string.
> 	* xml-syscall.h (get_syscall_names_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.

Heh, you're being too descriptive in the ChangeLog :-).  There is no
need to describe exactly what a function does, for example.  So you
could write:

  * xml-syscall.h (get_syscall_names_by_group): New prototype.

Anyway, this is just a comment to save you some time; I don't mind
having more information on the ChangeLog entry.

> ---
>  gdb/syscalls/gdb-syscalls.dtd |   1 +
>  gdb/xml-syscall.c             | 183 ++++++++++++++++++++++++++++++++++++++++--
>  gdb/xml-syscall.h             |  12 +++
>  3 files changed, 190 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/syscalls/gdb-syscalls.dtd b/gdb/syscalls/gdb-syscalls.dtd
> index 3ad3625..5255869 100644
> --- a/gdb/syscalls/gdb-syscalls.dtd
> +++ b/gdb/syscalls/gdb-syscalls.dtd
> @@ -10,5 +10,6 @@
>  
>  <!ELEMENT syscall		EMPTY>
>  <!ATTLIST syscall
> +	groups			CDATA	#OPTIONAL
>  	name			CDATA	#REQUIRED
>  	number			CDATA	#REQUIRED>

Just a nitpicking from me, but I'd prefer if you put "groups" below
"number", obeying the "natural" order of the fields inside the tag.

> diff --git a/gdb/xml-syscall.c b/gdb/xml-syscall.c
> index 3824468..39d3f02 100644
> --- a/gdb/xml-syscall.c
> +++ b/gdb/xml-syscall.c
> @@ -76,6 +76,20 @@ get_syscall_names (void)
>    return NULL;
>  }
>  
> +const char **
> +get_syscall_names_by_group (const char *group)
> +{
> +  syscall_warn_user ();
> +  return NULL;
> +}
> +
> +const char **
> +get_syscall_group_names (void)
> +{
> +  syscall_warn_user ();
> +  return NULL;
> +}

I know you are following the pattern you see in this file, and I truly
apologize because I wrote it, but I would love if you could put comments
on each new function you create in the file.  I really intend to do a
revamp on this file "soon", but meanwhile it would be great if new
additions to it followed our best coding style :-).

> +
>  #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);
> +}
> +
> +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,59 @@ make_cleanup_free_syscalls_info (struct syscalls_info *sysinfo)
>    return make_cleanup (free_syscalls_info, sysinfo);
>  }
>  
> +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 = strdup (group);

Use xstrdup here.

> +
> +  VEC_safe_push (syscall_group_desc_p, sysinfo->groups, groupdesc);
> +
> +  return groupdesc;
> +}
> +
> +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)
> +    {
> +      /* Create new group.  */
> +      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, ","))
> +      syscall_group_add_syscall (sysinfo, sysdesc, group);
> +
>    VEC_safe_push (syscall_desc_p, sysinfo->syscalls, sysdesc);
>  }
>  
> @@ -179,6 +269,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 +279,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 +295,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 +409,26 @@ init_sysinfo (void)
>    my_gdb_datadir = xstrdup (gdb_datadir);
>  }
>  
> +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;
> +
> +   /* 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)
> @@ -356,8 +470,10 @@ xml_get_syscall_name (const struct syscalls_info *sysinfo,
>  }
>  
>  static const char **
> -xml_list_of_syscalls (const struct syscalls_info *sysinfo)
> +xml_list_of_syscalls (const struct syscalls_info *sysinfo,
> +		      const char *group_filter)
>  {
> +  VEC(syscall_desc_p) *syscall_list = NULL;
>    struct syscall_desc *sysdesc;
>    const char **names = NULL;
>    int nsyscalls;
> @@ -366,11 +482,26 @@ xml_list_of_syscalls (const struct syscalls_info *sysinfo)
>    if (sysinfo == NULL)
>      return NULL;
>  
> -  nsyscalls = VEC_length (syscall_desc_p, sysinfo->syscalls);
> +  if (group_filter == NULL)
> +    {
> +      /* We use the general list that includes every syscall.  */
> +      syscall_list = sysinfo->syscalls;
> +    }
> +  else
> +    {
> +      struct syscall_group_desc *groupdesc;
> +
> +      groupdesc = syscall_group_get_group_by_name (sysinfo,
> +						   group_filter);
> +      if (groupdesc != NULL)
> +	syscall_list = groupdesc->syscalls;
> +    }

This part is a bit complicated.  For example, this last "if (groupdesc
!= NULL)" is telling me a bit of the design of the code.  IMO, when
groupdesc is NULL it means that you could not find a group whose name is
"group_filter".  In that case, you should return NULL to the caller,
since it asked for a group, and you could not find it.  However, you are
returning an allocated char ** whose first element is NULL.

What this all means to me is that you are somewhat exposing more than
you should, so the callers of this function should implement some
mechanisms to determine whether the group exists, or whether the user
specified a syscall name (see my future comment on the breakpoint.c
diff).  Anyway, this design is this way because you still haven't
implemented a way of explicitly specifying a syscall group to be caught.

> +
> +  nsyscalls = VEC_length (syscall_desc_p, syscall_list);
>    names = xmalloc ((nsyscalls + 1) * sizeof (char *));
>  
>    for (i = 0;
> -       VEC_iterate (syscall_desc_p, sysinfo->syscalls, i, sysdesc);
> +       VEC_iterate (syscall_desc_p, syscall_list, i, sysdesc);
>         i++)
>      names[i] = sysdesc->name;
>  
> @@ -379,6 +510,30 @@ xml_list_of_syscalls (const struct syscalls_info *sysinfo)
>    return names;
>  }
>  
> +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)
>  {
> @@ -410,7 +565,23 @@ get_syscall_names (void)
>  {
>    init_sysinfo ();
>  
> -  return xml_list_of_syscalls (sysinfo);
> +  return xml_list_of_syscalls (sysinfo, NULL);
> +}
> +
> +const char **
> +get_syscall_names_by_group (const char *group)
> +{
> +  init_sysinfo ();
> +
> +  return xml_list_of_syscalls (sysinfo, group);
> +}
> +
> +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..903a758 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.  If group is NULL, return every syscall. The vector is
> +   returned as an array of strings terminated by a NULL element.
> +   Returns the list of syscalls of a group, or NULL otherwise.  */
> +
> +const char **get_syscall_names_by_group (const char *group);
> +
> +/* 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

Other than the comment about group filtering, the patch looks good
enough to me.

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