Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC)

[Fabien Chouteau <chouteau@adacore.com>]

On 07/19/2013 07:19 PM, Scott Wood wrote:
> On 07/19/2013 04:22:46 AM, Fabien Chouteau wrote:
>> On 07/18/2013 10:37 PM, Scott Wood wrote:
>> > On 07/18/2013 04:27:50 AM, Fabien Chouteau wrote:
>> >> The BD is full, we will have to put the rest of padding in the next one.
>> >
>> > What rest of padding?  I thought you said rx_padding was 2 somehow?  If that were true, then it would be zero at the end.
>> >
>>
>> Read my description again.
>
> OK, I was confused by your answering "yes" to "wouldn't *size be 2
> here" -- I thought it was clear from the context that I was talking
> about at the time of the "if (*size == etsec->rx_padding)" statement.
> Maybe you thought I was talking about what would happen at that
> if-statement in the next descriptor?
>
> In any case, it's a bit hard to follow this code.

I agree it's a little bit hard, I will add some comments to make it
clearer.

> rx_fcb_size is included in to_write, but not in *size.

FCB is not considered as part of the packet, so not in remaining_data.

> rx_padding is included in *size, but not in to_write.

Padding on the other side is considered as part of the packet but not
allocated in the buffer.

> And it generally makes me nervous to see code that will go into an
> infinite loop (or other odd behavior) unless an exact-equality
> terminating condition is met, especially when it's as complicated as
> this, without an assertion to check for the bad case (even if it looks
> like it could never happen).
>

For me it is more confusing to see a less-or-equal when equal is the
necessary and sufficient condition. It will be even harder to understand
the code...

I will modify the code a bit, tell me if it's clearer.

Regards,

-- 
Fabien Chouteau