From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 4 Dec 2006 16:52:17 +1100 From: David Gibson To: Grant Likely Subject: Re: [LIBFDT] Fixup byteswapping code Message-ID: <20061204055217.GA32026@localhost.localdomain> References: <11650491414017-git-send-email-grant.likely@secretlab.ca> <20061204015329.GA4790@localhost.localdomain> <528646bc0612032118i689e9c83gdfc841a3d7b91fcf@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <528646bc0612032118i689e9c83gdfc841a3d7b91fcf@mail.gmail.com> Cc: linuxppc-dev@ozlabs.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Sun, Dec 03, 2006 at 10:18:25PM -0700, Grant Likely wrote: > On 12/3/06, David Gibson 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