[LIBFDT] Fixup byteswapping code

[David Gibson]

On Sun, Dec 03, 2006 at 10:18:25PM -0700, Grant Likely wrote:
> On 12/3/06, David Gibson <david at gibson.dropbear.id.au> wrote:
> > I just applied a patch with a whole batch of endian fixes, obsoleting
> > this patch of yours.
> 
> Cool, looks good and the tests pass on my machine now.
> 
> Now; here's an AMD64 related bug; consider line #44 in libfdt_internal.h:
> 
> #define PTR_ERROR(code)         (void *)(-(code))
> 
> Followed by line #67 in libfdt.h:
> 
> #define fdt_ptr_error(ptr) \
> 	( (((long)(ptr) < 0) && ((long)(ptr) >= -FDT_ERR_MAX)) ? -(long)(ptr) : 0 )
> 
> 
> 'code' being an integer.
> 
> This requires a hacky cast from int to pointer and back again, and it
> makes the assumption that int and void* are the same size.  However,
> on a x86_64 system (not sure about power64), int is 32 bits and void*
> is 64.  Therefore; a return of (void*)-1 results in address
> 0xffffffff; not 0xffffffffffffffff.  0xffffffff is right in the middle
> of valid address ranges.

Urg.  Yes indeed.  The same is true on powerpc64, but userspace is
usually 32-bits on powerpc64 machines; I hadn't tested compiling
64-bit.  An extra (long) in PTR_ERROR() appears to be enough to fix
it.

> This, of course, produces the warning: "cast to pointer from integer
> of different size"
> 
> I believe the accepted convention (at least in the kernel) is to
> return NULL on error, and libfdt should probably follow that
> convention.  

Actually, there are two conventions in the kernel.  Just returning
NULL on error is the most common.  But there's a significant number of
functions that encode an errno into the returned pointer in just this
way - see the IS_ERR(), PTR_ERR() and ERR_PTR() macros.

> If you must return the actual error code; maybe rework
> the API to accept a void** argument to be used to pass back the
> pointer and use 'return code', or include an 'int*' argument for
> passing back the err code.

I thought quite a bit about how to pass back errors here.  I'm not
very fond of the current method of encoding the error codes into the
return values - it's more magic than I'm comfortable with (especially
given the multiple encoding methods for different return types) - but
I haven't thought of a better way.

Having to use a void ** for the real return value is ugly - and means
you can't stack library calls together (result of one call as
parameter to another call).  Using int * for the error code is also
ugly and all too much encouragement for people not too check it.

What I'd really prefer is an errno-style interface, where a separate
variable or call must be consulted to find the details of the error.
But, I can't see any way to do that safely in code that's supposed to
be easy to import into a wide variety of execution environments
(kernel, zImage, bootloader, userspace) - some of which could be
multithreaded.

Oh incidentally, this isn't true at present, but I'd like to make it
true:  all the library functions should check their parameters so if
they're *given* one of the encoded error values, they'll just
propagate the error out again and won't do anything nasty.  That will
make it safe to nest library calls, even if the inner ones could fail.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson