RE: [PATCH 2/3] gdb: Add per-remote target variables for memory read and write config

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Andrew,

Thanks a lot for your feedback.
Before sending a V2 for this patch, there is still one point I would like to discuss
(refer to my answer to your comment below).

I also noticed that I missed an adaption in the function
static void
show_memory_packet_size (struct memory_packet_config *config)

at 
if (remote != NULL)
  printf_filtered (_("Packets are limited to %ld bytes.\n"),
		 remote->get_memory_packet_size (config));

In the function call get_memory_packet_size the global configuration is passed,
but the current target's configuration should be used.
I will correct this with the next version of this patch. 

> > The former global variables for that configuration are still available
> > to allow the command line configuration for all future remote
> > connections.  Similar to the command line configuration of the per-
> > remote target feature array, the commands
> >
> > - set remotewritesize (deprecated)
> > - set remote memory-read-packet-size
> > - set remote memory-write-packet-size
> >
> > will configure the current target (if available) and future remote
> > connections. The show command will display the global configuration
> > and the packet size of the current remote target, if available.
> >
> > It is required to adapt the test gdb.base/remote.exp which is failing
> > for --target_board=native-extended-gdbserver.  With that board GDB
> > connects to gdbserver at gdb start time.  Due to this patch two
> > loggings "The target may not be able to.." are shown if the command
> > 'set remote memory-write-packet-size fixed' is executed while a target
> > is connected for the current inferior.  To fix this, the clean_restart
> > command is moved to a later time point of the test.  It is sufficient
> > to be connected to the server when "runto_main" is executed.  Now the
> > connection time is similar to a testrun with
> > --target_board=native-gdbserver.
> >
> > To allow the user to distinguish between the packet-size configuration
> > for future connections and for the currently selected target, the
> > logging of the command 'set remote memory-write-packet-size fixed' is
> > adapted.
> > ---
> >  gdb/remote.c                      | 94 +++++++++++++++++++------------
> >  gdb/testsuite/gdb.base/remote.exp |  7 ++-
> >  2 files changed, 61 insertions(+), 40 deletions(-)
> 
> This needs a NEWS entry and a docs update.
> 
> Like with the previous patch, I think we should consider having the set/show
> commands print more informative messages about which targets are being
> changed or displayed.
> 

Yes, I agree, this could be improved.  I was wondering if we should then adapt
the show command such that it behaves similar to the previous patch e.g. 
"The show command always displays the current remote target's
configuration.  If no remote target is selected the default
configuration for future connections is shown.". 
For the GDB user it might be less confusing if we keep the behaviour of the
"show remote"  commands  consistent. 

> >
> > +  memory_packet_config m_memory_read_packet_config;
> > + memory_packet_config m_memory_write_packet_config;
> 
> Comment new fields please.

Yes, I will adapt it.

> > @@ -1956,9 +1971,16 @@ set_memory_packet_size (const char *args,
> struct memory_packet_config *config)
> >  			 ? DEFAULT_MAX_MEMORY_PACKET_SIZE_FIXED
> >  			 : size);
> >
> > -      if (! query (_("The target may not be able to correctly handle a %s\n"
> > -		   "of %ld bytes. Change the packet size? "),
> > -		   config->name, query_size))
> > +      if (target_connected
> > +	  && ! query (_("The target may not be able to correctly handle a
> %s\n"
> 
> The space after '!' is not gdb style, could you remove it please.
> Also the two below please.
> 
> Thanks,
> Andrew

Thank you, I will adapt it.

Best Regards,
Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928