[OpenBIOS] [PATCH] RFC: Change ofmem_common.c to set memory translation properties by reference

[Mark Cave-Ayland]

Andreas Färber wrote:

>> Blue/Andreas: please could you take a look at this patch and make sure 
>> it doesn't break anything in your SPARC64/PPC tests?
> 
> It looks as if none of us actually tested or ack'ed it yet, all answers 
> were about sparc progress and devices only...
> Me for one couldn't save your inline patch and my mailer is known to 
> cause whitespace damage, and git-am wasn't able to handle the mbox file 
> since your SVN diff is missing the a/ directory level expected by Git.
> 
> If you're asking for comments from us, please be more patient. I'm still 
> playing with Blue's slightly older libgcc patch, and some trivial ones 
> of mine (e.g., CONFIG_RTAS or wrong return) are awaiting 
> review/committing longer than yours. Please at least let us know in 
> advance how long you're gonna wait, so that we have a chance to send in 
> incomplete review comments even if still untested.

I feel your pain when it comes to having multiple patchsets on the go at 
once. For a while, I ended up with about 3 simultaneous sets working 
various things that had bugs, which when I fixed had other other bugs...

In terms of the patch comments, maybe I could have waited longer. 
Generally the OpenBIOS code is still being actively developed, and so 
the few people actively working on the code generally push on with what 
they are doing, and as long as people don't introduce regressions then 
no-one complains too loudly.

A few times when I've been unsure about a patch, I've posted it to the 
list for comment and people have been fairly quick in the past to 
criticise any bad points, so mostly I accept lack of response that 
people are generally happy with it. Perhaps now with more people working 
on the code we should reconsider this? I also did a complete set of 
testing on my PPC test images and didn't see any regressions there, so I 
felt it was reasonably safe...

> I like the general optimization idea, but I found one typo s/fix/fit/ 
> and I was wondering whether the property setting function really needs 
> to live inside ofmem_common.c or whether we might want to make 
> set_property_nocopy() out of it for general use.

Yes possibly, although I think strings/integers need to be encoded (and 
it is the encoding process that makes the copy of the input) whereas 
natively the properties work using byte arrays and so we can just set 
the property to point directly to them.

> I also had some style questions related to the inconsistent use of 
> spacing inside braces. Some ppc code even has a wild mix of tabs and 8 
> spaces, which is unfortunate when many other projects including QEMU use 
> an indentation of 4. We're lacking a CODING_STYLE document imo, to look 
> up which way it's supposed to be.

Yes, maybe we should start thinking more about CODING_STYLE...

> BR,
> Andreas
> 
> P.S. I have local patches to prepare ofmem_release() inside ofmem_common.c.

Interesting - these would be very useful :)


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: +44 870 608 0063

Sirius Labs: http://www.siriusit.co.uk/labs