[RFA] Factor out some functions in remote.c

[RFA] Factor out some functions in remote.c

[Michael Snyder]

These are two little functions that factor out the conversion between
ascii-encoded hex and binary, and vice versa, something that seems to
be done inline over and over in remote.c (and something that I was 
about to do inline yet another time).
2001-05-02  Michael Snyder  <msnyder@redhat.com>

	* remote.c (bin2hex, hex2bin): New functions.  Factor out these
	two conversions which are coded for repeatedly in this module.
	(remote_threads_extra_info, remote_wait, remote_async_wait, 
	store_register_using_P, remote_store_registers, remote_write_bytes,
	remote_read_bytes, remote_rcmd): Use bin2hex and hex2bin instead
	of coding the conversions inline.
	(fromhex): Not exported, change from extern to static.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.46
diff -c -3 -p -r1.46 remote.c
*** remote.c	2001/04/26 22:10:41	1.46
--- remote.c	2001/05/02 21:34:57
*************** static void remote_find_new_threads (voi
*** 183,191 ****
  
  static void record_currthread (int currthread);
  
! /* exported functions */
  
! extern int fromhex (int a);
  
  static int putpkt_binary (char *buf, int cnt);
  
--- 183,191 ----
  
  static void record_currthread (int currthread);
  
! static int fromhex (int a);
  
! static int hex2bin (char *, char *, int);
  
  static int putpkt_binary (char *buf, int cnt);
  
*************** static void show_packet_config_cmd (stru
*** 197,202 ****
--- 197,204 ----
  
  static void update_packet_config (struct packet_config *config);
  
+ /* exported functions */
+ 
  /* Define the target subroutine names */
  
  void open_remote_target (char *, int, struct target_ops *, int);
*************** remote_threads_extra_info (struct thread
*** 1729,1745 ****
        getpkt (bufp, PBUFSIZ, 0);
        if (bufp[0] != 0)
  	{
! 	  char *p;
! 
! 	  for (p = display_buf; 
! 	       p < display_buf + sizeof(display_buf) - 1 &&
! 		 bufp[0] != 0 &&
! 		 bufp[1] != 0;
! 	       p++, bufp+=2)
! 	    {
! 	      *p = fromhex (bufp[0]) * 16 + fromhex (bufp[1]);
! 	    }
! 	  *p = 0;
  	  return display_buf;
  	}
      }
--- 1731,1738 ----
        getpkt (bufp, PBUFSIZ, 0);
        if (bufp[0] != 0)
  	{
! 	  n = min (strlen (bufp) / 2, sizeof (display_buf));
! 	  display_buf [hex2bin (bufp, display_buf, n)] = '\0';
  	  return display_buf;
  	}
      }
*************** remote_async_detach (char *args, int fro
*** 2316,2322 ****
  
  /* Convert hex digit A to a number.  */
  
! int
  fromhex (int a)
  {
    if (a >= '0' && a <= '9')
--- 2309,2315 ----
  
  /* Convert hex digit A to a number.  */
  
! static int
  fromhex (int a)
  {
    if (a >= '0' && a <= '9')
*************** fromhex (int a)
*** 2329,2334 ****
--- 2322,2350 ----
      error ("Reply contains invalid hex digit %d", a);
  }
  
+ static int
+ hex2bin (char *hex, char *bin, int count)
+ {
+   int i;
+ 
+   /* May use a length, or a nul-terminated string as input. */
+   if (count == 0)
+     count = strlen (hex) / 2;
+ 
+   for (i = 0; i < count; i++)
+     {
+       if (hex[0] == 0 || hex[1] == 0)
+ 	{
+ 	  /* Hex string is short, or of uneven length.
+ 	     Return the count that has been converted so far. */
+ 	  return i;
+ 	}
+       *bin++ = fromhex (hex[0]) * 16 + fromhex (hex[1]);
+       hex += 2;
+     }
+   return i;
+ }
+ 
  /* Convert number NIB to a hex digit.  */
  
  static int
*************** tohex (int nib)
*** 2339,2344 ****
--- 2355,2377 ----
    else
      return 'a' + nib - 10;
  }
+ 
+ static int
+ bin2hex (char *bin, char *hex, int count)
+ {
+   int i;
+   /* May use a length, or a nul-terminated string as input. */
+   if (count == 0)
+     count = strlen (bin);
+ 
+   for (i = 0; i < count; i++)
+     {
+       *hex++ = tohex ((*bin >> 4) & 0xf);
+       *hex++ = tohex (*bin++ & 0xf);
+     }
+   *hex = 0;
+   return i;
+ }
  \f
  /* Tell the remote machine to resume.  */
  
*************** Packet: '%s'\n",
*** 2822,2834 ****
  Packet: '%s'\n",
  			       regno, p, buf);
  
! 		    for (i = 0; i < REGISTER_RAW_SIZE (regno); i++)
! 		      {
! 			if (p[0] == 0 || p[1] == 0)
! 			  warning ("Remote reply is too short: %s", buf);
! 			regs[i] = fromhex (p[0]) * 16 + fromhex (p[1]);
! 			p += 2;
! 		      }
  		    supply_register (regno, regs);
  		  }
  
--- 2855,2863 ----
  Packet: '%s'\n",
  			       regno, p, buf);
  
! 		    if (hex2bin (p, regs, REGISTER_RAW_SIZE (regno))
! 			< REGISTER_RAW_SIZE (regno))
! 		      warning ("Remote reply is too short: %s", buf);
  		    supply_register (regno, regs);
  		  }
  
*************** Packet: '%s'\n",
*** 3043,3055 ****
  Packet: '%s'\n",
  			       regno, p, buf);
  
! 		    for (i = 0; i < REGISTER_RAW_SIZE (regno); i++)
! 		      {
! 			if (p[0] == 0 || p[1] == 0)
! 			  warning ("Remote reply is too short: %s", buf);
! 			regs[i] = fromhex (p[0]) * 16 + fromhex (p[1]);
! 			p += 2;
! 		      }
  		    supply_register (regno, regs);
  		  }
  
--- 3072,3080 ----
  Packet: '%s'\n",
  			       regno, p, buf);
  
! 		    if (hex2bin (p, regs, REGISTER_RAW_SIZE (regno)) 
! 			< REGISTER_RAW_SIZE (regno))
! 		      warning ("Remote reply is too short: %s", buf);
  		    supply_register (regno, regs);
  		  }
  
*************** store_register_using_P (int regno)
*** 3299,3310 ****
    sprintf (buf, "P%x=", regno);
    p = buf + strlen (buf);
    regp = register_buffer (regno);
!   for (i = 0; i < REGISTER_RAW_SIZE (regno); ++i)
!     {
!       *p++ = tohex ((regp[i] >> 4) & 0xf);
!       *p++ = tohex (regp[i] & 0xf);
!     }
!   *p = '\0';
    remote_send (buf, PBUFSIZ);
  
    return buf[0] != '\0';
--- 3324,3330 ----
    sprintf (buf, "P%x=", regno);
    p = buf + strlen (buf);
    regp = register_buffer (regno);
!   bin2hex (regp, p, REGISTER_RAW_SIZE (regno));
    remote_send (buf, PBUFSIZ);
  
    return buf[0] != '\0';
*************** remote_store_registers (int regno)
*** 3361,3373 ****
    regs = register_buffer (-1);
    p = buf + 1;
    /* remote_prepare_to_store insures that register_bytes_found gets set.  */
!   for (i = 0; i < register_bytes_found; i++)
!     {
!       *p++ = tohex ((regs[i] >> 4) & 0xf);
!       *p++ = tohex (regs[i] & 0xf);
!     }
!   *p = '\0';
! 
    remote_send (buf, PBUFSIZ);
  }
  \f
--- 3381,3387 ----
    regs = register_buffer (-1);
    p = buf + 1;
    /* remote_prepare_to_store insures that register_bytes_found gets set.  */
!   bin2hex (regs, p, register_bytes_found);
    remote_send (buf, PBUFSIZ);
  }
  \f
*************** remote_write_bytes (CORE_ADDR memaddr, c
*** 3594,3605 ****
        /* Normal mode: Send target system values byte by byte, in
  	 increasing byte addresses.  Each byte is encoded as a two hex
  	 value.  */
!       for (nr_bytes = 0; nr_bytes < todo; nr_bytes++)
! 	{
! 	  *p++ = tohex ((myaddr[nr_bytes] >> 4) & 0xf);
! 	  *p++ = tohex (myaddr[nr_bytes] & 0xf);
! 	}
!       *p = '\0';
        break;
      case PACKET_SUPPORT_UNKNOWN:
        internal_error (__FILE__, __LINE__,
--- 3608,3614 ----
        /* Normal mode: Send target system values byte by byte, in
  	 increasing byte addresses.  Each byte is encoded as a two hex
  	 value.  */
!       bin2hex (myaddr, p, todo);
        break;
      case PACKET_SUPPORT_UNKNOWN:
        internal_error (__FILE__, __LINE__,
*************** remote_read_bytes (CORE_ADDR memaddr, ch
*** 3690,3703 ****
           each byte encoded as two hex characters.  */
  
        p = buf;
!       for (i = 0; i < todo; i++)
  	{
! 	  if (p[0] == 0 || p[1] == 0)
! 	    /* Reply is short.  This means that we were able to read
! 	       only part of what we wanted to.  */
! 	    return i + (origlen - len);
! 	  myaddr[i] = fromhex (p[0]) * 16 + fromhex (p[1]);
! 	  p += 2;
  	}
        myaddr += todo;
        memaddr += todo;
--- 3699,3709 ----
           each byte encoded as two hex characters.  */
  
        p = buf;
!       if ((i = hex2bin (p, myaddr, todo)) < todo)
  	{
! 	  /* Reply is short.  This means that we were able to read
! 	     only part of what we wanted to. */
! 	  return i + (origlen - len);
  	}
        myaddr += todo;
        memaddr += todo;
*************** remote_rcmd (char *command,
*** 4897,4908 ****
      error ("\"monitor\" command ``%s'' is too long\n", command);
  
    /* Encode the actual command */
!   for (i = 0; command[i]; i++)
!     {
!       *p++ = tohex ((command[i] >> 4) & 0xf);
!       *p++ = tohex (command[i] & 0xf);
!     }
!   *p = '\0';
  
    if (putpkt (buf) < 0)
      error ("Communication problem with target\n");
--- 4903,4909 ----
      error ("\"monitor\" command ``%s'' is too long\n", command);
  
    /* Encode the actual command */
!   bin2hex (command, p, 0);
  
    if (putpkt (buf) < 0)
      error ("Communication problem with target\n");

Re: [RFA] Factor out some functions in remote.c

[Andrew Cagney]

> These are two little functions that factor out the conversion between
> ascii-encoded hex and binary, and vice versa, something that seems to
> be done inline over and over in remote.c (and something that I was 
> about to do inline yet another time).

Mostly ok.  Just some tweeks.


> + /* exported functions */
> + 

this comment can be deleted.


> --- 1731,1738 ----
>         getpkt (bufp, PBUFSIZ, 0);
>         if (bufp[0] != 0)
>   	{
> ! 	  n = min (strlen (bufp) / 2, sizeof (display_buf));
> ! 	  display_buf [hex2bin (bufp, display_buf, n)] = '\0';
>   	  return display_buf;
>   	}

I needed several takes on this one.  Can you split the line:

  	  display_buf [hex2bin (bufp, display_buf, n)] = '\0';

into two steps?


>       }
> *************** remote_async_detach (char *args, int fro
> *** 2316,2322 ****
>   
>   /* Convert hex digit A to a number.  */
>   
> ! int
>   fromhex (int a)
>   {
>     if (a >= '0' && a <= '9')
> --- 2309,2315 ----
>   
>   /* Convert hex digit A to a number.  */
>   
> ! static int
>   fromhex (int a)
>   {
>     if (a >= '0' && a <= '9')
> *************** fromhex (int a)
> *** 2329,2334 ****
> --- 2322,2350 ----
>       error ("Reply contains invalid hex digit %d", a);
>   }
>   
> + static int
> + hex2bin (char *hex, char *bin, int count)

suggest ``const char *hex''.


> + {
> +   int i;
> + 
> +   /* May use a length, or a nul-terminated string as input. */
> +   if (count == 0)
> +     count = strlen (hex) / 2;

Could I suggest leaving this out.  Looking through the code, this 
doesn't appear to be necessary and it leaves the function open to a 
buffer overrun.


>     p = buf + strlen (buf);
>     regp = register_buffer (regno);
> !   bin2hex (regp, p, REGISTER_RAW_SIZE (regno));
>     remote_send (buf, PBUFSIZ);

perhaphs write this as:

	p += bin2hex (regp, p, ...);
	*p = '\0';


so it is clear that the buffer was null terminated.

	nice cleanup,
		Andrew

Re: [RFA] Factor out some functions in remote.c

[Michael Snyder]

Andrew Cagney wrote:
> 
> > These are two little functions that factor out the conversion between
> > ascii-encoded hex and binary, and vice versa, something that seems to
> > be done inline over and over in remote.c (and something that I was
> > about to do inline yet another time).
> 
> Mostly ok.  Just some tweeks.
> 
> > + /* exported functions */
> > +
> 
> this comment can be deleted.

OK.

> > --- 1731,1738 ----
> >         getpkt (bufp, PBUFSIZ, 0);
> >         if (bufp[0] != 0)
> >       {
> > !       n = min (strlen (bufp) / 2, sizeof (display_buf));
> > !       display_buf [hex2bin (bufp, display_buf, n)] = '\0';
> >         return display_buf;
> >       }
> 
> I needed several takes on this one.  Can you split the line:
> 
>           display_buf [hex2bin (bufp, display_buf, n)] = '\0';
> 
> into two steps?

OK.


> >       }
> > *************** remote_async_detach (char *args, int fro
> > *** 2316,2322 ****
> >
> >   /* Convert hex digit A to a number.  */
> >
> > ! int
> >   fromhex (int a)
> >   {
> >     if (a >= '0' && a <= '9')
> > --- 2309,2315 ----
> >
> >   /* Convert hex digit A to a number.  */
> >
> > ! static int
> >   fromhex (int a)
> >   {
> >     if (a >= '0' && a <= '9')
> > *************** fromhex (int a)
> > *** 2329,2334 ****
> > --- 2322,2350 ----
> >       error ("Reply contains invalid hex digit %d", a);
> >   }
> >
> > + static int
> > + hex2bin (char *hex, char *bin, int count)
> 
> suggest ``const char *hex''.

OK.

> > + {
> > +   int i;
> > +
> > +   /* May use a length, or a nul-terminated string as input. */
> > +   if (count == 0)
> > +     count = strlen (hex) / 2;
> 
> Could I suggest leaving this out.  Looking through the code, this
> doesn't appear to be necessary and it leaves the function open to a
> buffer overrun.

No, I need it.  I just haven't submitted the new code that needs it yet.
I think fixed length buffers and null terminated strings are both reasonable
input.  If you want me to code two separate functions, one for fixed length
buffers and one for null terminated strings, I will (but I'll be sad).

> >     p = buf + strlen (buf);
> >     regp = register_buffer (regno);
> > !   bin2hex (regp, p, REGISTER_RAW_SIZE (regno));
> >     remote_send (buf, PBUFSIZ);
> 
> perhaphs write this as:
> 
>         p += bin2hex (regp, p, ...);
>         *p = '\0';
> 
> so it is clear that the buffer was null terminated.

The output of bin2hex is a string, and it's always null terminated.
I don't think it's reasonable for the caller to have to null terminate
it every time it is called, when the callee can (and should) do it.

I'll submit a new patch based on your feedback.

Michael

Re: [RFA] Factor out some functions in remote.c

[Andrew Cagney]

> No, I need it.  I just haven't submitted the new code that needs it yet.
> I think fixed length buffers and null terminated strings are both reasonable
> input.  If you want me to code two separate functions, one for fixed length
> buffers and one for null terminated strings, I will (but I'll be sad).

This relates bach to strncpy() vs strlcpy() and things like asprintf() 
vs sprintf().  If you've not heard of it strlcpy() takes a parameter 
that determines the size of the _destionation_ buffer and not the source 
buffer.  This dramatically reduced the chance of buffer overruns.

I would prefer that these conversion functions always took a parameter 
that determined the size of the destination buffer.  If nothing else, 
could you code call it as something like hex2bin(src,dest,strlen(src))?

	Andrew

Re: [RFA] Factor out some functions in remote.c

[Michael Snyder]

Andrew Cagney wrote:
> 
> > No, I need it.  I just haven't submitted the new code that needs it yet.
> > I think fixed length buffers and null terminated strings are both reasonable
> > input.  If you want me to code two separate functions, one for fixed length
> > buffers and one for null terminated strings, I will (but I'll be sad).
> 
> This relates bach to strncpy() vs strlcpy() and things like asprintf()
> vs sprintf().  If you've not heard of it strlcpy() takes a parameter
> that determines the size of the _destionation_ buffer and not the source
> buffer.  This dramatically reduced the chance of buffer overruns.
> 
> I would prefer that these conversion functions always took a parameter
> that determined the size of the destination buffer.  If nothing else,
> could you code call it as something like hex2bin(src,dest,strlen(src))?

Yes, OK, I'll make that change.