[PATCH] zero-terminate result of target_read_alloc

[PATCH] zero-terminate result of target_read_alloc

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]


This patch makes result of target_read_alloc zero-terminated.
The point is that often the object is not allowed to contain embedded zeros,
and working with zero-terminated strings is much easier.

OK?

- Volodya

2006-07-18  Vladimir Prus  <vladimir@codesourcery.com>

	* target.c (target_read_alloc): Zero-terminate result.


[-- Attachment #2: 2_target_read_alloc_zero_terminate.diff --]
[-- Type: text/x-diff, Size: 1449 bytes --]

--- target.c	(revision 174)
+++ target.c	(revision 175)
@@ -1413,6 +1413,11 @@ target_write (struct target_ops *ops,
    sufficiently large buffer will be allocated using xmalloc and
    returned in *BUF_P containing the contents of the object.
 
+   The content will be zero terminated.  The caller should consider 
+   if object can legitimately contain embedded zero, and either use 
+   string operations, or use the returned object length when 
+   manupulating the buffer.
+
    This method should be used for objects sufficiently small to store
    in a single xmalloc'd buffer, when no fixed bound on the object's
    size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
@@ -1442,7 +1447,7 @@ target_read_alloc (struct target_ops *op
   while (1)
     {
       n = target_read_partial (ops, object, annex, &buf[buf_pos],
-			       buf_pos, buf_alloc - buf_pos);
+			       buf_pos, buf_alloc - buf_pos - 1);
       if (n < 0)
 	{
 	  /* An error occurred.  */
@@ -1455,14 +1460,17 @@ target_read_alloc (struct target_ops *op
 	  if (buf_pos == 0)
 	    xfree (buf);
 	  else
-	    *buf_p = buf;
+            {
+              buf[buf_pos] = '\0';
+              *buf_p = buf;
+            }
 	  return buf_pos;
 	}
 
       buf_pos += n;
 
       /* If the buffer is filling up, expand it.  */
-      if (buf_alloc < buf_pos * 2)
+      if (buf_pos >= buf_alloc - 1)
 	{
 	  buf_alloc *= 2;
 	  buf = xrealloc (buf, buf_alloc);

Re: [PATCH] zero-terminate result of target_read_alloc

[Mark Kettenis]

>
>  This patch makes result of target_read_alloc zero-terminated.
>  The point is that often the object is not allowed to contain embedded
>  zeros,
>  and working with zero-terminated strings is much easier.
>
>  OK?

This is wrong.  Either the terminating nul is part of the object you're
reading or it is not.  GDB shouldn't at its own.

>
>  - Volodya
>
>  2006-07-18  Vladimir Prus  <vladimir@codesourcery.com>
>
>  	* target.c (target_read_alloc): Zero-terminate result.
>
>

Re: [PATCH] zero-terminate result of target_read_alloc

[Vladimir Prus]

On Tuesday 18 July 2006 15:25, Mark Kettenis wrote:
> >  This patch makes result of target_read_alloc zero-terminated.
> >  The point is that often the object is not allowed to contain embedded
> >  zeros,
> >  and working with zero-terminated strings is much easier.
> >
> >  OK?
>
> This is wrong.  Either the terminating nul is part of the object you're
> reading or it is not.  GDB shouldn't at its own.

I'm sorry, I don't understand the problem. Some objects are binary in nature, 
can contain zeroes, and should be manipulated using length.

Some objects, for example XML memory map, are text ones. Embedded zero byte in 
such object is bug in remote side, and nothing else. And while it would not 
be overly complicate to pass length of such objects in my case, I don't see 
why we should impose this complication on everybody.

- Volodya

Re: [PATCH] zero-terminate result of target_read_alloc

[Daniel Jacobowitz]

On Tue, Jul 18, 2006 at 01:25:22PM +0200, Mark Kettenis wrote:
> >
> >  This patch makes result of target_read_alloc zero-terminated.
> >  The point is that often the object is not allowed to contain embedded
> >  zeros,
> >  and working with zero-terminated strings is much easier.
> >
> >  OK?
> 
> This is wrong.  Either the terminating nul is part of the object you're
> reading or it is not.  GDB shouldn't at its own.

I figured the same but actually reading a couple of objects and
managing them convinced me that Volodya's right.  Note that he's not
adding one to the length returned - if you want binary data, you'll get
binary data.  It just allows you to easily treat the result as a string
if you plan to treat it as a string anyway.

As you can probably guess from context, we have these bunches of XML
files that we read from the target and then do text processing on. 
On the branch where I developed the XML bits for the first time, I
didn't think to make this change - so the very first thing I had to do
with the result of target_read_alloc was to allocate something one byte
bigger with a NUL on the end of it!

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] zero-terminate result of target_read_alloc

[Mark Kettenis]

>  On Tue, Jul 18, 2006 at 01:25:22PM +0200, Mark Kettenis wrote:
> > >
> > >  This patch makes result of target_read_alloc zero-terminated.
> > >  The point is that often the object is not allowed to contain embedded
> > >  zeros,
> > >  and working with zero-terminated strings is much easier.
> > >
> > >  OK?
> >
> > This is wrong.  Either the terminating nul is part of the object you're
> > reading or it is not.  GDB shouldn't at its own.
>
>  I figured the same but actually reading a couple of objects and
>  managing them convinced me that Volodya's right.  Note that he's not
>  adding one to the length returned - if you want binary data, you'll get
>  binary data.  It just allows you to easily treat the result as a string
>  if you plan to treat it as a string anyway.

Sorry, but the whole distinction between strings and binary data is wrong.
Strings are binary data, including the terminating NUL.  Volodya's patch
addresses things at the wrong level.  Whatever we read using
target_read_partial() should already include the terminating NUL.

>  As you can probably guess from context, we have these bunches of XML
>  files that we read from the target and then do text processing on.
>  On the branch where I developed the XML bits for the first time, I
>  didn't think to make this change - so the very first thing I had to do
>  with the result of target_read_alloc was to allocate something one byte
>  bigger with a NUL on the end of it!

So whatever reads the XML from the target should make sure things are
NUL-terminated, and report the total length, including the terminating
NUL.

I guess you're thinking too much in terms of the remote protocol, where
you need to make the distinction between text and binary data because
you have to encode the latter to avoid problems with embedded NUL's.  But
that's really specific to the way our remote protocol works, and therefore
should be handled completely in the remote target vector code.

Mark

Re: [PATCH] zero-terminate result of target_read_alloc

[Daniel Jacobowitz]

On Tue, Jul 18, 2006 at 03:14:59PM +0200, Mark Kettenis wrote:
> Sorry, but the whole distinction between strings and binary data is wrong.
> Strings are binary data, including the terminating NUL.  Volodya's patch
> addresses things at the wrong level.  Whatever we read using
> target_read_partial() should already include the terminating NUL.

Please consider that you are asserting that a design change is obvious
and necessary in code that we've been working with for months.  I
disagree with you that it's at the wrong level; please at least listen
to why.

> So whatever reads the XML from the target should make sure things are
> NUL-terminated, and report the total length, including the terminating
> NUL.
> 
> I guess you're thinking too much in terms of the remote protocol, where
> you need to make the distinction between text and binary data because
> you have to encode the latter to avoid problems with embedded NUL's.  But
> that's really specific to the way our remote protocol works, and therefore
> should be handled completely in the remote target vector code.

In fact it's exactly the opposite.  Embedded NULs are not a problem
with the remote protocol.  It is perfectly happy to include NUL
characters in the data.  The characters which cause a problem are
[*$#].  Text and binary data are not at all distinguished in the remote
protocol interface underlying TARGET_OBJECT_*.  Both are escaped in the
same way, which used to be as hex, but is now a binary encoding only
changing those special characters.

The remote protocol doesn't care at all what's in the object.  Right
now it doesn't know.  Later on, it will know the contents of some
objects (e.g. the XML memory map) and parse them - but not inside
to_xfer_partial, which is strictly a transfer mechanism.  That would be
a layering problem.  Instead other bits of the file will call
target_read_alloc to fetch objects.

Here's a better way to think about the patch, I think.  Consider the
result of target_read_alloc to be the contents of a disk file.  It
might be an ELF executable or a text README file.  The caller knows
which sort of data it is, and will process it appropriately, but
target_read_alloc is just fread in this model.  It doesn't know whether
the contents are text or binary.  If they are text, why should they
include a terminating NUL in the disk file?

So with this change, the interface is friendly to consumers which wish
to parse the result as binary data, and also friendly to consumers
which wish to pass it to strcpy or strlen.  Yes, I realize my analogy
is a bit flawed in that fread doesn't do this.  But how frequently have
you seen fread followed by some code to add a NUL at the end of the
buffer?  I checked a handful of source trees I had handy, and the
answer seems to be about 30% of the time; pretty substantial, and these
aren't text-io-heavy applications, e.g. gcc/libcpp.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] zero-terminate result of target_read_alloc

[Vladimir Prus]

On Tuesday 18 July 2006 17:39, Daniel Jacobowitz wrote:

> Here's a better way to think about the patch, I think.  Consider the
> result of target_read_alloc to be the contents of a disk file.  It
> might be an ELF executable or a text README file.  The caller knows
> which sort of data it is, and will process it appropriately, but
> target_read_alloc is just fread in this model.  It doesn't know whether
> the contents are text or binary.  If they are text, why should they
> include a terminating NUL in the disk file?
>
> So with this change, the interface is friendly to consumers which wish
> to parse the result as binary data, and also friendly to consumers
> which wish to pass it to strcpy or strlen.  Yes, I realize my analogy
> is a bit flawed in that fread doesn't do this.  

I think this is quite reasonable analogy. remote packet is just like a fread. 
packet. target_read_alloc is like some higher-level code, say to read a value 
from config file, or a read entire file into memory. In both cases, the disk 
file don't have terminating nulls and you get zero-terminated char*.

If target_read_alloc does not zero-terminate data, it requires either manually 
zero-terminating result, or drag length everywhere. And while it's possible 
for a remote side to include extra zero byte and increase the size of data by 
one, that's a fairly uncommon interface, and is likely to result in buggy 
stubs. 

- Volodya

Re: [PATCH] zero-terminate result of target_read_alloc

[Jim Blandy]

Vladimir Prus <vladimir@codesourcery.com> writes:
> If target_read_alloc does not zero-terminate data, it requires
> either manually zero-terminating result, or drag length
> everywhere. And while it's possible for a remote side to include
> extra zero byte and increase the size of data by one, that's a
> fairly uncommon interface, and is likely to result in buggy stubs.

Another way to look at it is to say that target_read_alloc clearly
operates on blocks of uninterpreted bytes --- it returns address and
length, after all --- but the interface as given requires users
retrieving text that they'd like to process as null-terminated strings
to make an extra copy.  The change to the interface Vlad's proposed
would make the copy unnecessary.

Of course we can't accomodate every random string representation
anyone could come up with; we can't avoid copies in every case.  And
it's not our job to do so.  But null-termination is a common need, and
one actually at hand.

Examples from elsewhere: in Guile, even though our strings could
contain arbitrary binary data, we also included an extra null byte at
the end, beyond the official length of the string, so that C code that
knew it was working with text (filenames, for example) could simply
use the string contents in place, without needing to make a
null-terminated copy.

Re: [PATCH] zero-terminate result of target_read_alloc

[Daniel Jacobowitz]

On Tue, Jul 18, 2006 at 01:25:22PM +0200, Mark Kettenis wrote:
> >
> >  This patch makes result of target_read_alloc zero-terminated.
> >  The point is that often the object is not allowed to contain embedded
> >  zeros,
> >  and working with zero-terminated strings is much easier.
> >
> >  OK?
> 
> This is wrong.  Either the terminating nul is part of the object you're
> reading or it is not.  GDB shouldn't at its own.

Hi Mark,

There was some followup discussion on this, and we didn't hear back
from you.  My own explanation of the current behavior is that this is
an interface for reading binary data from the target, much like reading
it from files, so it shouldn't be NUL terminated - but the client may
expect the data to not contain embedded NULs and we have the
opportunity to be helpful here, so we should be helpful.

Do you find that convincing?  If not, would you be happier if there
were two functions to do this, one which added the NUL and one which
did not?  I'm thinking target_read_alloc and target_read_stralloc,
indicating that we allocate the result as if it were a string.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] zero-terminate result of target_read_alloc

[Mark Kettenis]

> Date: Mon, 24 Jul 2006 00:09:37 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Tue, Jul 18, 2006 at 01:25:22PM +0200, Mark Kettenis wrote:
> > >
> > >  This patch makes result of target_read_alloc zero-terminated.
> > >  The point is that often the object is not allowed to contain embedded
> > >  zeros,
> > >  and working with zero-terminated strings is much easier.
> > >
> > >  OK?
> > 
> > This is wrong.  Either the terminating nul is part of the object you're
> > reading or it is not.  GDB shouldn't at its own.
> 
> Hi Mark,
> 
> There was some followup discussion on this, and we didn't hear back
> from you.  My own explanation of the current behavior is that this is
> an interface for reading binary data from the target, much like reading
> it from files, so it shouldn't be NUL terminated - but the client may
> expect the data to not contain embedded NULs and we have the
> opportunity to be helpful here, so we should be helpful.

Sorry, yes.  I didn't manage to reply yet.  I don't find the arguments
very convincing.  You try to justify the interface by giving examples
of interfaces that are very different from target_read_alloc() (and
much more similar to target_read()).

> Do you find that convincing?  If not, would you be happier if there
> were two functions to do this, one which added the NUL and one which
> did not?  I'm thinking target_read_alloc and target_read_stralloc,
> indicating that we allocate the result as if it were a string.

That seems a reasonably compromise to me.  It makes things much more
explicit.  I think you should go one step further, and make
target_read_stralloc() return "char *" instead of "LONGEST" (and do
away with the "gdb_byte **" argument).  That'd solve the issue whether
the length returned includes the terminating NUL or not.  The function
would return a NULL pointer upon failure and an empty string on (the
equivalent of) EOF.

Mark

Re: [PATCH] zero-terminate result of target_read_alloc

[Daniel Jacobowitz]

On Mon, Jul 24, 2006 at 11:38:25PM +0200, Mark Kettenis wrote:
> Sorry, yes.  I didn't manage to reply yet.  I don't find the arguments
> very convincing.  You try to justify the interface by giving examples
> of interfaces that are very different from target_read_alloc() (and
> much more similar to target_read()).

Well, it's hardly _my_ fault that the C library doesn't offer
fread_whole_file! :-)

> > Do you find that convincing?  If not, would you be happier if there
> > were two functions to do this, one which added the NUL and one which
> > did not?  I'm thinking target_read_alloc and target_read_stralloc,
> > indicating that we allocate the result as if it were a string.
> 
> That seems a reasonably compromise to me.  It makes things much more
> explicit.  I think you should go one step further, and make
> target_read_stralloc() return "char *" instead of "LONGEST" (and do
> away with the "gdb_byte **" argument).  That'd solve the issue whether
> the length returned includes the terminating NUL or not.  The function
> would return a NULL pointer upon failure and an empty string on (the
> equivalent of) EOF.

All right!  Compromise and progress!  Thanks, Mark.

How about this patch (untested so far)?  My only concern is that it
doesn't report error/unsupported separately from empty; but for all the
uses I know of so far, that's not a problem, and we can change it later
if we need to.

-- 
Daniel Jacobowitz
CodeSourcery

2006-07-24  Daniel Jacobowitz  <dan@codesourcery.com>

	* target.h (target_read_stralloc): New prototype.
	* target.c (target_read_alloc_1): Renamed from target_read_alloc.
	Take new PADDING argument.
	(target_read_alloc): Use it.
	(target_read_stralloc): New function.

Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.85
diff -u -p -r1.85 target.h
--- target.h	12 Jul 2006 18:13:45 -0000	1.85
+++ target.h	24 Jul 2006 21:59:03 -0000
@@ -236,6 +236,14 @@ extern LONGEST target_read_alloc (struct
 				  enum target_object object,
 				  const char *annex, gdb_byte **buf_p);
 
+/* Read OBJECT/ANNEX using OPS.  The result is NUL-terminated
+   and returned as a string.  A warning is issued if the result
+   contains any embedded NUL bytes.  */
+
+extern char *target_read_stralloc (struct target_ops *ops,
+				   enum target_object object,
+				   const char *annex);
+
 /* Wrappers to target read/write that perform memory transfers.  They
    throw an error if the memory transfer fails.
 
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.121
diff -u -p -r1.121 target.c
--- target.c	18 Jul 2006 12:44:48 -0000	1.121
+++ target.c	24 Jul 2006 21:59:03 -0000
@@ -1406,22 +1406,15 @@ target_write (struct target_ops *ops,
   return len;
 }
 
-/* Wrapper to perform a full read of unknown size.  OBJECT/ANNEX will
-   be read using OPS.  The return value will be -1 if the transfer
-   fails or is not supported; 0 if the object is empty; or the length
-   of the object otherwise.  If a positive value is returned, a
-   sufficiently large buffer will be allocated using xmalloc and
-   returned in *BUF_P containing the contents of the object.
-
-   This method should be used for objects sufficiently small to store
-   in a single xmalloc'd buffer, when no fixed bound on the object's
-   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
-   through this function.  */
-
-LONGEST
-target_read_alloc (struct target_ops *ops,
-		   enum target_object object,
-		   const char *annex, gdb_byte **buf_p)
+/* Read OBJECT/ANNEX using OPS.  Store the result in *BUF_P and return
+   the size of the transferred data.  PADDING additional bytes are
+   available in *BUF_P.  This is a helper function for
+   target_read_alloc; see the declaration of that function for more
+   information.  */
+
+static LONGEST
+target_read_alloc_1 (struct target_ops *ops, enum target_object object,
+		     const char *annex, gdb_byte **buf_p, int padding)
 {
   size_t buf_alloc, buf_pos;
   gdb_byte *buf;
@@ -1442,7 +1435,7 @@ target_read_alloc (struct target_ops *op
   while (1)
     {
       n = target_read_partial (ops, object, annex, &buf[buf_pos],
-			       buf_pos, buf_alloc - buf_pos);
+			       buf_pos, buf_alloc - buf_pos - padding);
       if (n < 0)
 	{
 	  /* An error occurred.  */
@@ -1472,6 +1465,41 @@ target_read_alloc (struct target_ops *op
     }
 }
 
+/* Read OBJECT/ANNEX using OPS.  Store the result in *BUF_P and return
+   the size of the transferred data.  See the declaration in "target.h"
+   function for more information about the return value.  */
+
+LONGEST
+target_read_alloc (struct target_ops *ops, enum target_object object,
+		   const char *annex, gdb_byte **buf_p)
+{
+  return target_read_alloc_1 (ops, object, annex, buf_p, 0);
+}
+
+/* Read OBJECT/ANNEX using OPS.  The result is NUL-terminated
+   and returned as a string.  A warning is issued if the result
+   contains any embedded NUL bytes.  */
+
+char *
+target_read_stralloc (struct target_ops *ops, enum target_object object,
+		      const char *annex)
+{
+  gdb_byte *buffer;
+  LONGEST transferred;
+
+  transferred = target_read_alloc_1 (ops, object, annex, &buffer, 1);
+
+  if (transferred <= 0)
+    return NULL;
+
+  buffer[transferred] = 0;
+  if (strlen (buffer) < transferred)
+    warning (_("target object %d, annex %s, contained unexpected zero bytes"),
+	     (int) object, annex ? annex : "(none)");
+
+  return (char *) buffer;
+}
+
 /* Memory transfer methods.  */
 
 void

Re: [PATCH] zero-terminate result of target_read_alloc

[Mark Kettenis]

> Date: Mon, 24 Jul 2006 18:00:43 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> All right!  Compromise and progress!  Thanks, Mark.
> 
> How about this patch (untested so far)?  My only concern is that it
> doesn't report error/unsupported separately from empty; but for all the
> uses I know of so far, that's not a problem, and we can change it later
> if we need to.

Better get this right from the start, and return xstrdup("") for
"empty".  Also the error message

"... contained unexpected zero bytes"

is not very clear; the first time I thought that would warn about
returning an empty string.  How about changing that to

"... contained unexpected null characters"

?  

The C standard uses the term "null character" for a byte with all bits
set to 0.

> 2006-07-24  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* target.h (target_read_stralloc): New prototype.
> 	* target.c (target_read_alloc_1): Renamed from target_read_alloc.
> 	Take new PADDING argument.
> 	(target_read_alloc): Use it.
> 	(target_read_stralloc): New function.
> 
> Index: target.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.h,v
> retrieving revision 1.85
> diff -u -p -r1.85 target.h
> --- target.h	12 Jul 2006 18:13:45 -0000	1.85
> +++ target.h	24 Jul 2006 21:59:03 -0000
> @@ -236,6 +236,14 @@ extern LONGEST target_read_alloc (struct
>  				  enum target_object object,
>  				  const char *annex, gdb_byte **buf_p);
>  
> +/* Read OBJECT/ANNEX using OPS.  The result is NUL-terminated
> +   and returned as a string.  A warning is issued if the result
> +   contains any embedded NUL bytes.  */
> +
> +extern char *target_read_stralloc (struct target_ops *ops,
> +				   enum target_object object,
> +				   const char *annex);
> +
>  /* Wrappers to target read/write that perform memory transfers.  They
>     throw an error if the memory transfer fails.
>  
> Index: target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.121
> diff -u -p -r1.121 target.c
> --- target.c	18 Jul 2006 12:44:48 -0000	1.121
> +++ target.c	24 Jul 2006 21:59:03 -0000
> @@ -1406,22 +1406,15 @@ target_write (struct target_ops *ops,
>    return len;
>  }
>  
> -/* Wrapper to perform a full read of unknown size.  OBJECT/ANNEX will
> -   be read using OPS.  The return value will be -1 if the transfer
> -   fails or is not supported; 0 if the object is empty; or the length
> -   of the object otherwise.  If a positive value is returned, a
> -   sufficiently large buffer will be allocated using xmalloc and
> -   returned in *BUF_P containing the contents of the object.
> -
> -   This method should be used for objects sufficiently small to store
> -   in a single xmalloc'd buffer, when no fixed bound on the object's
> -   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
> -   through this function.  */
> -
> -LONGEST
> -target_read_alloc (struct target_ops *ops,
> -		   enum target_object object,
> -		   const char *annex, gdb_byte **buf_p)
> +/* Read OBJECT/ANNEX using OPS.  Store the result in *BUF_P and return
> +   the size of the transferred data.  PADDING additional bytes are
> +   available in *BUF_P.  This is a helper function for
> +   target_read_alloc; see the declaration of that function for more
> +   information.  */
> +
> +static LONGEST
> +target_read_alloc_1 (struct target_ops *ops, enum target_object object,
> +		     const char *annex, gdb_byte **buf_p, int padding)
>  {
>    size_t buf_alloc, buf_pos;
>    gdb_byte *buf;
> @@ -1442,7 +1435,7 @@ target_read_alloc (struct target_ops *op
>    while (1)
>      {
>        n = target_read_partial (ops, object, annex, &buf[buf_pos],
> -			       buf_pos, buf_alloc - buf_pos);
> +			       buf_pos, buf_alloc - buf_pos - padding);
>        if (n < 0)
>  	{
>  	  /* An error occurred.  */
> @@ -1472,6 +1465,41 @@ target_read_alloc (struct target_ops *op
>      }
>  }
>  
> +/* Read OBJECT/ANNEX using OPS.  Store the result in *BUF_P and return
> +   the size of the transferred data.  See the declaration in "target.h"
> +   function for more information about the return value.  */
> +
> +LONGEST
> +target_read_alloc (struct target_ops *ops, enum target_object object,
> +		   const char *annex, gdb_byte **buf_p)
> +{
> +  return target_read_alloc_1 (ops, object, annex, buf_p, 0);
> +}
> +
> +/* Read OBJECT/ANNEX using OPS.  The result is NUL-terminated
> +   and returned as a string.  A warning is issued if the result
> +   contains any embedded NUL bytes.  */
> +
> +char *
> +target_read_stralloc (struct target_ops *ops, enum target_object object,
> +		      const char *annex)
> +{
> +  gdb_byte *buffer;
> +  LONGEST transferred;
> +
> +  transferred = target_read_alloc_1 (ops, object, annex, &buffer, 1);
> +
> +  if (transferred <= 0)
> +    return NULL;
> +
> +  buffer[transferred] = 0;
> +  if (strlen (buffer) < transferred)
> +    warning (_("target object %d, annex %s, contained unexpected zero bytes"),
> +	     (int) object, annex ? annex : "(none)");
> +
> +  return (char *) buffer;
> +}
> +
>  /* Memory transfer methods.  */
>  
>  void
> 

Re: [PATCH] zero-terminate result of target_read_alloc

[Daniel Jacobowitz]

On Wed, Jul 26, 2006 at 11:51:24PM +0200, Mark Kettenis wrote:
> > Date: Mon, 24 Jul 2006 18:00:43 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > All right!  Compromise and progress!  Thanks, Mark.
> > 
> > How about this patch (untested so far)?  My only concern is that it
> > doesn't report error/unsupported separately from empty; but for all the
> > uses I know of so far, that's not a problem, and we can change it later
> > if we need to.
> 
> Better get this right from the start, and return xstrdup("") for
> "empty".  Also the error message

... Why didn't I think of that?  Brilliant!

> "... contained unexpected zero bytes"
> 
> is not very clear; the first time I thought that would warn about
> returning an empty string.  How about changing that to
> 
> "... contained unexpected null characters"
> 
> ?  
> 
> The C standard uses the term "null character" for a byte with all bits
> set to 0.

That works for me.  OK with those changes?


-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] zero-terminate result of target_read_alloc

[Mark Kettenis]

> Date: Wed, 26 Jul 2006 18:25:46 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Wed, Jul 26, 2006 at 11:51:24PM +0200, Mark Kettenis wrote:
> > > Date: Mon, 24 Jul 2006 18:00:43 -0400
> > > From: Daniel Jacobowitz <drow@false.org>
> > > 
> > > All right!  Compromise and progress!  Thanks, Mark.
> > > 
> > > How about this patch (untested so far)?  My only concern is that it
> > > doesn't report error/unsupported separately from empty; but for all the
> > > uses I know of so far, that's not a problem, and we can change it later
> > > if we need to.
> > 
> > Better get this right from the start, and return xstrdup("") for
> > "empty".  Also the error message
> 
> ... Why didn't I think of that?  Brilliant!
> 
> > "... contained unexpected zero bytes"
> > 
> > is not very clear; the first time I thought that would warn about
> > returning an empty string.  How about changing that to
> > 
> > "... contained unexpected null characters"
> > 
> > ?  
> > 
> > The C standard uses the term "null character" for a byte with all bits
> > set to 0.
> 
> That works for me.  OK with those changes?

Sure

Re: [PATCH] zero-terminate result of target_read_alloc

[Daniel Jacobowitz]

On Thu, Jul 27, 2006 at 12:34:06AM +0200, Mark Kettenis wrote:
> Sure

Checked in as so.

-- 
Daniel Jacobowitz
CodeSourcery

2006-07-27  Daniel Jacobowitz  <dan@codesourcery.com>

	* target.h (target_read_stralloc): New prototype.
	* target.c (target_read_alloc_1): Renamed from target_read_alloc.
	Take new PADDING argument.
	(target_read_alloc): Use it.
	(target_read_stralloc): New function.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.121
diff -u -p -r1.121 target.c
--- target.c	18 Jul 2006 12:44:48 -0000	1.121
+++ target.c	27 Jul 2006 21:23:13 -0000
@@ -1406,22 +1406,15 @@ target_write (struct target_ops *ops,
   return len;
 }
 
-/* Wrapper to perform a full read of unknown size.  OBJECT/ANNEX will
-   be read using OPS.  The return value will be -1 if the transfer
-   fails or is not supported; 0 if the object is empty; or the length
-   of the object otherwise.  If a positive value is returned, a
-   sufficiently large buffer will be allocated using xmalloc and
-   returned in *BUF_P containing the contents of the object.
-
-   This method should be used for objects sufficiently small to store
-   in a single xmalloc'd buffer, when no fixed bound on the object's
-   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
-   through this function.  */
-
-LONGEST
-target_read_alloc (struct target_ops *ops,
-		   enum target_object object,
-		   const char *annex, gdb_byte **buf_p)
+/* Read OBJECT/ANNEX using OPS.  Store the result in *BUF_P and return
+   the size of the transferred data.  PADDING additional bytes are
+   available in *BUF_P.  This is a helper function for
+   target_read_alloc; see the declaration of that function for more
+   information.  */
+
+static LONGEST
+target_read_alloc_1 (struct target_ops *ops, enum target_object object,
+		     const char *annex, gdb_byte **buf_p, int padding)
 {
   size_t buf_alloc, buf_pos;
   gdb_byte *buf;
@@ -1442,7 +1435,7 @@ target_read_alloc (struct target_ops *op
   while (1)
     {
       n = target_read_partial (ops, object, annex, &buf[buf_pos],
-			       buf_pos, buf_alloc - buf_pos);
+			       buf_pos, buf_alloc - buf_pos - padding);
       if (n < 0)
 	{
 	  /* An error occurred.  */
@@ -1472,6 +1465,47 @@ target_read_alloc (struct target_ops *op
     }
 }
 
+/* Read OBJECT/ANNEX using OPS.  Store the result in *BUF_P and return
+   the size of the transferred data.  See the declaration in "target.h"
+   function for more information about the return value.  */
+
+LONGEST
+target_read_alloc (struct target_ops *ops, enum target_object object,
+		   const char *annex, gdb_byte **buf_p)
+{
+  return target_read_alloc_1 (ops, object, annex, buf_p, 0);
+}
+
+/* Read OBJECT/ANNEX using OPS.  The result is NUL-terminated and
+   returned as a string, allocated using xmalloc.  If an error occurs
+   or the transfer is unsupported, NULL is returned.  Empty objects
+   are returned as allocated but empty strings.  A warning is issued
+   if the result contains any embedded NUL bytes.  */
+
+char *
+target_read_stralloc (struct target_ops *ops, enum target_object object,
+		      const char *annex)
+{
+  gdb_byte *buffer;
+  LONGEST transferred;
+
+  transferred = target_read_alloc_1 (ops, object, annex, &buffer, 1);
+
+  if (transferred < 0)
+    return NULL;
+
+  if (transferred == 0)
+    return xstrdup ("");
+
+  buffer[transferred] = 0;
+  if (strlen (buffer) < transferred)
+    warning (_("target object %d, annex %s, "
+	       "contained unexpected null characters"),
+	     (int) object, annex ? annex : "(none)");
+
+  return (char *) buffer;
+}
+
 /* Memory transfer methods.  */
 
 void
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.85
diff -u -p -r1.85 target.h
--- target.h	12 Jul 2006 18:13:45 -0000	1.85
+++ target.h	27 Jul 2006 21:23:13 -0000
@@ -236,6 +236,16 @@ extern LONGEST target_read_alloc (struct
 				  enum target_object object,
 				  const char *annex, gdb_byte **buf_p);
 
+/* Read OBJECT/ANNEX using OPS.  The result is NUL-terminated and
+   returned as a string, allocated using xmalloc.  If an error occurs
+   or the transfer is unsupported, NULL is returned.  Empty objects
+   are returned as allocated but empty strings.  A warning is issued
+   if the result contains any embedded NUL bytes.  */
+
+extern char *target_read_stralloc (struct target_ops *ops,
+				   enum target_object object,
+				   const char *annex);
+
 /* Wrappers to target read/write that perform memory transfers.  They
    throw an error if the memory transfer fails.