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

Sérgio Durigan Júnior
2008-11-07T03:46:59Z

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