This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/4] 'catch syscall' feature -- XML support part
- From: Tom Tromey <tromey at redhat dot com>
- To: SÃrgio Durigan JÃnior <sergiodj at linux dot vnet dot ibm dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 04 Nov 2008 11:40:43 -0700
- Subject: Re: [PATCH 3/4] 'catch syscall' feature -- XML support part
- References: <1225773086.24532.55.camel@miki>
- Reply-to: tromey at redhat dot com
>>>>> "SÃrgio" == SÃrgio Durigan JÃnior <sergiodj@linux.vnet.ibm.com> writes:
SÃrgio> This is the part which adds XML support for the "catch
SÃrgio> syscall" feature. I decided to put the architecture's XML
SÃrgio> files here too, so that the architecture-independent part does
SÃrgio> not grow too much.
Just a few nits from me...
SÃrgio> +struct syscalls_info
SÃrgio> +{
SÃrgio> + /* The number of syscalls in this system. */
SÃrgio> +
SÃrgio> + int number_of_syscalls;
SÃrgio> +
SÃrgio> + /* The syscalls. */
SÃrgio> +
SÃrgio> + VEC(syscall_desc_p) *syscalls;
SÃrgio> +};
I think number_of_syscalls is redundant here. The VEC knows how many
elements it contains, so why not just use that?
SÃrgio> +static void
SÃrgio> +do_cleanup_fclose (void *file)
SÃrgio> +{
SÃrgio> + fclose (file);
SÃrgio> +}
There's a make_cleanup_fclose now.
SÃrgio> + <syscall name="" number="223"/>
[...]
SÃrgio> + for (i = 0; i < len; i++)
SÃrgio> + {
SÃrgio> + if (strcmp (attrs[i].name, "name") == 0)
SÃrgio> + name = attrs[i].value;
SÃrgio> + else if (strcmp (attrs[i].name, "number") == 0)
SÃrgio> + number = * (ULONGEST *) attrs[i].value;
SÃrgio> + else
SÃrgio> + internal_error (__FILE__, __LINE__,
SÃrgio> + _("Unknown attribute name '%s'."), attrs[i].name);
SÃrgio> + }
SÃrgio> +
SÃrgio> + syscall_create_syscall_desc (data->sysinfo, name, number);
This will make an entry with an empty name given the above XML
snippet. But that doesn't seem helpful... if this triggers, won't it
print as an unnamed syscall? It seems to me that it would be
preferable to skip nameless ones and just report them numerically, if
they occur.
SÃrgio> +static const struct gdb_xml_attribute syscall_attr[] = {
SÃrgio> + { "number", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
The indentation is wrong here and in other definitions like this.
It should be just 2 spaces.
SÃrgio> +static struct syscalls_info *
SÃrgio> +syscall_parse_xml (const char *document, xml_fetch_another fetcher,
SÃrgio> + void *fetcher_baton)
SÃrgio> +{
[...]
SÃrgio> + back_to = make_cleanup (null_cleanup, NULL);
I don't think you need to make a null cleanup here...
SÃrgio> + if (gdb_xml_parse (parser, document) == 0)
SÃrgio> + {
SÃrgio> + /* Parsed successfully. */
SÃrgio> + discard_cleanups (result_cleanup);
SÃrgio> + do_cleanups (back_to);
SÃrgio> + return data.sysinfo;
SÃrgio> + }
SÃrgio> + else
SÃrgio> + {
SÃrgio> + warning (_("Could not load XML syscalls info; ignoring"));
SÃrgio> + do_cleanups (back_to);
SÃrgio> + return NULL;
... you can either just do_ or discard_ result_cleanup instead.
Tom