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

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

Corinna Vinschen wrote:
> - The idea to redirect all error messages to the wrapper call in
>   xdr_private.c is a good idea in general.  I don't think you should
>   have mixed feelings about it.

OK.

> - *Only* for Cygwin I would prefer to keep the default behaviour
>   to print to stderr, since that's what glibc is doing as well.
>   The redirection to debug_printf is not necessary.

I see. OK.

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

OK, will do.

>> 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...
> 
> Yes, that sounds good.

Alright.

>>> In xdr_double, replace BYTE_ORDER == BIG_ENDIAN with __IEEE_BIG_ENDIAN.
>> Do you mean
>> -#if BYTE_ORDER == BIG_ENDIAN
>> +#ifdef __IEEE_BIG_ENDIAN
>> ?
> 
> Yes, that was the idea.  This and __IEEE_LITTLE_ENDIAN are used throughout
> newlib/libm code so it makes sense to reuse them here.

Ack.

BTW, you said earlier:
> 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.

Do you mean I should /remove/ all of the vax support from xdr_float.c?

>> I'll present all of these requested
>> changes as an interdiff, for easier review.
> 
> Just a single patch with everything would be ok, too, in this case.

Well, these changes are all in "new" files, so it'd be hard to see what
I've changed between the original submission and the update, if I just
regenerated "the" patch. (Maybe we're both saying the same thing: I plan
to show a diff between [original submission, pretend it was already
applied to newlib/] and [the new version] -- e.g. a sequential patch
that could be applied on-top-of the original submission).

>> 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.
> 
> The stdio stuff is only pulled in if the application actually uses the
> functions from xdrstdio.c, and if the application uses them, it needs
> stdio.  So I don't see how that hurts.

I agree with respect to the possibly undefined functions used by
xdr_stdio.c like fwrite, fread, fseek, etc.

But I was more concerned with the declaration of the FILE type (aka
__FILE).  Is it always universally declared by stdio.h regardless of
platform, even if stdio/ is not built? (<sys/reent.h> is a maze of
twistly little #ifdefs).

That having been said, it just seems...unclean...to plan on compiling
library objects that have no chance of linking properly, even if doing
so would not actually hurt anything.

The NetBSD version of xdr.h guards the declaration of xdrstdio_create()
with #ifdef _STDIO_H_, and doesn't include <stdio.h> -- so you don't get
the declaration of xdrstdio_create() unless you #include <stdio.h>
yourself, before #including <rpc/xdr.h>. (I didn't look further to see
how they handle xdrstdio.c).  I like this; it avoids polluting the
public rpc/xdr.h header with HAVE_* or WANT_* or (e.g.) __CYGWIN__ macros...

The FreeBSD version has removed the stdio-related routines entirely,
replacing them with versions that operate on "struct mbuf*" (some sort
of linked list of memory buffers used inside the FreeBSD kernel).

I've also just noticed that a lot of the .c's in "my" implementation
#include <stdio.h>, but they don't need to (probably just inherited from
the old Sun RPC, or maybe because I was using printf-style debugging at
one point or another). I'll fix that.

Given the various changes discussed, I think it will be fairly easy to
ensure that the ONLY file that unconditionally includes stdio.h and uses
its routines is xdrstdio.c.  And if that file is excluded from the build
by the pre-existing HAVE_STDIO_DIR AM_CONDITIONAL...it'd be much cleaner
for targets that don't have stdio.

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

OK.

--
Chuck