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

[Tom Tromey <tromey@redhat.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