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

[Corinna Vinschen <vinschen at redhat dot com>]

Hi Chuck,

thanks for the patch.  I didn't look much into the actual code yet,
but I have a few points.

On Feb 25 00:05, Charles Wilson wrote:
> Error reporting:
> ================
> As mentioned above, most XDR implementations use the BSDish warnx
> function to immediately print the (very rare) error messages to stderr.
> Similarly, here error messages are printed to stderr by default --
> except that a framework is provided for the caller to register a
> different callback function for error reporting. (The cygwin glue, not
> included here, registers a callback that redirects errors to cygwin's
> debug strace facility).
> 
> This registration function is not declared in the public headers; I
> expect that, if any platform other than cygwin "turns on" xdr, will also
> declare and auto-register an appropriate callback, here inside newlib,
> so it can simply include xdr_private.h to get the necessary decls.

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.

> 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.

More general, the error reporting is simply part of the code so I'd just
keep it in.  Adding a way to turn it into something else is a good idea
for smaller targets and code running on some embedded board.

> 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.

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)'.
In xdr_double, replace BYTE_ORDER == BIG_ENDIAN with __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.

Would you mind to do that?

> add some intelligence to optionally exclude those limited portions of
> the XDR code that require stdio to work...

I think that's sufficiently covered.

> implementation is 90% of the way there for those platforms (and 100% of
> the way there for cygwin).  As submitted though, XDR is completely
> "turned off" for all but $host cygwin.
> 
> I recognize that embedded targets can sometimes be picky about
> implementation details (is malloc() allowed? are stdio streams
> available?, etc).  As provided here, XDR uses both.  However, xdr (and

Well, stdio is used only in xdr_private.c, so, see above.  As for malloc,
that seems to be nicely wrapped by mem_alloc and mem_free macros, so
targets can override them as needed (README?).

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?

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


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat