Re: Add XDR support (only built #if cygwin, for now)

[Charles Wilson <cygwin at cwilson dot fastmail dot fm>]

Corinna Vinschen wrote:
> Wouldn't it make sense to add a README to the xdr directory which
> describes what to do to port XDR to another target?  A few words
> about where and how to add the callback function would be quite helpful.

Sure.

>> I'm of mixed feelings about this whole error reporting issue: all of the
>> error messages, inherited from the original SunRPC code, are "out of
>> memory" errors. Perhaps it would be better to avoid reporting these
>> messages to any console/stream, and instead silently return failure with
>> errno=ENOMEM?  OTOH, the SunRPC implementation is almost universal...
> 
> Talking about Cygwin: I'm a bit puzzled why it's necessary to convert
> the stderr printing into debug_printf messages at all.  The error
> reporting to stderr is simply part of the code.  Even the glibc code
> is still doing it unconditionally.

But most embedded targets avoid glibc like the plague.  In general it's
a really bad idea for libraries to print error messages -- doing so
could interfere with the actual application's HMI.  OTOH, if there is
any library that could be said to "own" the standard IO streams, and
thus be allowed to inject its own error messages into them, it's the C
runtime library...

> More general, the error reporting is simply part of the code so I'd just
> keep it in.  

(a) IMHO that was a mistake in the original implementation (b) we can
certainly customize the implementation -- and, to a limited extent,
peripheral behavioral issues like this -- for newlib's purposes.

> Adding a way to turn it into something else is a good idea
> for smaller targets and code running on some embedded board.

Hmmm...I begin to believe, for better harmony with embedded targets,
it'd be better to default to /nothing/ (except for setting errno) unless
the client has specifically registered an appropriate callback for those
error messages.

Now, if you like, perhaps cygwin should register a callback that simply
prints to stderr (rather than debug strace), so that for cygwin the XDR
functions act "just like" glibc's...

For those frequent occasions where we run out of memory. (It's actually
pointless to specifically direct XDR's out-of-memory messages to the
debug strace, because I'm pretty sure cygwin's malloc would already have
indicated the out-of-memory on that debug strace anyway...)


Alternatively, I could leave in the fprintf(), but simply guard it with
#if HAVE_STDIO (or whatever).  That'd be more in line with the glibc
implementation, with only the absolute minimal necessary accommodation
to resource-limited embedded targets.  But...I don't like it. <g>

>> Applicability to newlib patforms other than cygwin
>> ==================================================
>> At present, XDR is not built unless $host==cygwin.  I figure other
>> interested parties can later activate it when they put in whatever
>> minimal effort may be required to get it to compile for them. (It's
>> pretty dirt-simple C code; build system integration -- already done --
>> and header file prereqs are actually the hard part).  Other targets
>> might need to #ifdef out support for e.g. long longs or doubles; maybe
> 
> Disabling support for the 64 bit integral datatypes should be very simple
> indeed.  The XDR code uses int64_t and u_int64_t.  Newlib's headers
> define ___int64_t_defined in that case.  So, just disabling the code
> using the int64_t and u_int64_t datatypes and the derived quad_t and
> u_quad_t based on ___int64_t_defined should do it.

OK. That should be easy enough.

> As for double, I'm not at all fluent with libm stuff, but it looks like
> tweaking the code for newlib is not overly complicated.
> If _DOUBLE_IS_32BITS is set, call xdr_float from xdr_double.
> Replace `#ifdef IEEEFP' with `#if defined (__IEEE_LITTLE_ENDIAN) ||
> defined (__IEEE_BIG_ENDIAN)'.

OK.

> In xdr_double, replace BYTE_ORDER == BIG_ENDIAN with __IEEE_BIG_ENDIAN.

Do you mean
-#if BYTE_ORDER == BIG_ENDIAN
+#ifdef __IEEE_BIG_ENDIAN
?

> Last but not least, if none of __IEEE_LITTLE_ENDIAN and __IEEE_BIG_ENDIAN
> is defined, disable float support entirely *for now*.  The code in
> xdr_float.c only handles IEEEFP or VAX floats.  Other formats require
> some additional target-specific code.

OK.

> Would you mind to do that?

Not a problem.  It may take a while, and I won't actually be able to
test it, except artificially.  I'll present all of these requested
changes as an interdiff, for easier review.

>> add some intelligence to optionally exclude those limited portions of
>> the XDR code that require stdio to work...
> 
> I think that's sufficiently covered.
> 
> Well, stdio is used only in xdr_private.c, so, see above.

And maybe not even there, if the by-default-only-errno route is chosen.
However, xdrstdio.[h|c] definitely uses stdio, as do the
xdrstdio_create() and related functions declared in xdr.h (FILE*
arguments).  So those need to be guarded.

I'd have to use both a macro (to hide the appropriate portions of xdr.h)
and an automake AM_CONDITIONAL (to exclude xdrstdio.c from the build).
But I think all of the necessary such symbols are already available.

> As for malloc,
> that seems to be nicely wrapped by mem_alloc and mem_free macros, so
> targets can override them as needed (README?).

Ack.

Should that part of rpc/types.h be updated?

-#define mem_alloc(bsize)        calloc(1, bsize)
-#define mem_free(ptr, bsize)    free(ptr)
+#ifndef mem_alloc
+#define mem_alloc(bsize)        calloc(1, bsize)
+#endif
+#ifndef mem_free
+#define mem_free(ptr, bsize)    free(ptr)
+#endif

> However, I see that malloc and free are used in xdr_sizeof.c, rather than
> mem_alloc and mem_free.  Shouldn't that be fixed?

That's from the inherited Sun RPC code, but I agree it should be fixed.

> With the above changes I don't actually see a reason to restrict the
> XDR build to Cygwin.  

Weeeellll, maybe not -- but it ain't gonna be ME that activates a giant
chunk of new code for 173 different platforms without testing them!

Doing the work described above may make it *easier* for target
maintainers to simply add xdr in configure.host and it might "just
work", but that ought to be their decision, not mine.

Or yours and Jeff's, after actual testing on at least a few of those
targets...

--
Chuck