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

[Sérgio Durigan Júnior <sergiodj at linux dot vnet dot ibm dot com>]

Hey again :-)

On Tue, 2008-11-04 at 11:40 -0700, Tom Tromey wrote:
> >>>>> "Sérgio" == Sérgio Durigan Júnior <sergiodj@linux.vnet.ibm.com> writes:
> 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?

Hmm, OK. I'm getting used with this VEC data structure, so I still don't
know what it does/doesn't provide. Thanks for the hint.

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

Didn't know too. Thanks.

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

I'm sorry, but I think I didn't understand completely what you
suggested. You mean that I should set the syscall name to NULL when
there's no name for it in the XML file?

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

Ha, I was sure that I'd receive some complains about the
indentation :-P.

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

Right.

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

I'm sorry, instead of what? do_cleanups? :-)

Thanks again!

Regards,

-- 
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil