Re: [Qemu-devel] [PATCH v2] spapr: Add "memop" hypercall

[Alexander Graf]

On 25.05.2012, at 10:36, Benjamin Herrenschmidt <address@hidden> wrote:

> On Fri, 2012-05-25 at 10:30 +0200, Alexander Graf wrote:
> 
>>> +    while (count--) {
>>> +        switch (esize) {
>>> +        case 0: tmp = ldub_phys(src);
>> 
>> I'm surprised checkpatch didn't complain here. Please do
>> 
>> case x:
>>    foo();
>>    break();
>> 
>>> break;
>>> +        case 1: tmp = lduw_phys(src); break;
>>> +        case 2: tmp = ldl_phys(src);  break;
>>> +        case 3: tmp = ldq_phys(src);  break;
>>> +        default:
>>> +        return H_PARAMETER;
> 
> Checkpatch absolutely complained and I decided to ignore it, seriously,
> you really want to replace a nice & readable piece of code with
> something that takes 3 pages and is generally gross & ugly ?
> 
> Some times, you have to ignore check patch and let sanity prevail.

I'm not all that keen on coding style rules. But check out 
arch/powerpc/kvm/emulate.c and tell me that it's a good idea to go with this 
"clean" approach. If you want it really clean, put the whole chunk above into a 
geberic helper that allows for everyone to say "read n bytes of data with 
native endianness into a u64". In that code, the more verbose coding style 
checkpatch suggests doesn't hurt and your function becomes even easier to read 
:)

> 
> Ben.
> 
>> Indentation?
> 
> Not sure what's up with identation, I had it all fixed up to please
> checkpatch, maybe I screwed up the sending of the patch itself.

It could be my mailer too, no idea :). Just stumbled over it.

> Oh
> well, I'm off to hospital on monday so that will have to wait til I'm
> back (I regret you didn't make those comments on the previous iteration
> of the patch though).

Yeah, it's a shame I didn't read through it more thoroughly earlier - at least 
it didn't take weeks in this round ;).

No worries though, if you can't make it until Monday, I'll fix it up myself 
afterwards :). There's no black magic involved here, so I should be ok to 
respin myself.


Alex