Re: [PATCH 04/12] record btrace: add configuration struct

[Pedro Alves <palves@redhat.com>]

On 07/14/2014 02:56 PM, Markus Metzger wrote:

> index 146d95f..c601e8d 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -131,6 +131,9 @@ struct vstop_notif
>    struct target_waitstatus status;
>  };
>
> +/* The current btrace configuration.  */
> +static struct btrace_config current_btrace_conf;

Why do we need this global?  Please expand the comment.

> --- a/gdb/remote.c
> +++ b/gdb/remote.c

> +/* The target's btrace configuration.  */
> +
> +static struct btrace_config btrace_target_config;

If this mirrors the state on the remote target, then
it should be a field of struct remote_state, not global.

> +/* Synchronize the configuration with the target.  */
> +
> +static void btrace_sync_conf (const struct btrace_config *conf)

Line break after void.

> +{
> +  /* Nothing to do for now.  */
> +}
> +
> +/* Read the current thread's btrace configuration from the target and
> +   store it into CONF.  */
> +
> +static void btrace_read_config (struct btrace_config *conf)
> +{

Line break after void.

> +static void btrace_read_config (struct btrace_config *conf)
> +{
> +  struct cleanup *cleanup;

Move this within the if block below.

> +  char *xml;
> +
> +  xml = target_read_stralloc (&current_target,
> +                              TARGET_OBJECT_BTRACE_CONF, "");
> +  if (xml != NULL)
> +    {
> +
> +      cleanup = make_cleanup (xfree, xml);
> +      parse_xml_btrace_conf (conf, xml);

Spurious empty line.


Thanks,
Pedro Alves