Re: [PATCH] zero-terminate result of target_read_alloc

[Daniel Jacobowitz <drow at false dot org>]

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