From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22848 invoked by alias); 14 Nov 2014 22:42:35 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 22796 invoked by uid 89); 14 Nov 2014 22:42:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 14 Nov 2014 22:42:32 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAEMgSET029637 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 14 Nov 2014 17:42:28 -0500 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sAEMgR69014862 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Fri, 14 Nov 2014 17:42:28 -0500 From: Sergio Durigan Junior To: Gabriel Krisman Bertazi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/4] Implemement support for groups of syscalls in the xml-syscall interface. References: <1414956944-8856-1-git-send-email-gabriel@krisman.be> <1414956944-8856-2-git-send-email-gabriel@krisman.be> X-URL: http://blog.sergiodj.net Date: Fri, 14 Nov 2014 22:42:00 -0000 In-Reply-To: <1414956944-8856-2-git-send-email-gabriel@krisman.be> (Gabriel Krisman Bertazi's message of "Sun, 2 Nov 2014 17:35:41 -0200") Message-ID: <87y4rd76b0.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg00340.txt.bz2 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 @@ > > 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/