This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 3/4] 'catch syscall' feature -- XML support part


>>>>> "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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]