[Qemu-devel] Re: [PATCH] eepro100: Restructure code (new function tx_command)

[Stefan Weil]

Michael S. Tsirkin schrieb:
> On Sat, Dec 12, 2009 at 08:45:06PM +0100, Stefan Weil wrote:
>> Michael S. Tsirkin schrieb:
>>> On Mon, Nov 30, 2009 at 06:05:31PM +0100, Stefan Weil wrote:
>>>> Michael S. Tsirkin schrieb:
>>>>> On Thu, Nov 19, 2009 at 09:54:46PM +0100, Stefan Weil wrote:
>>>>>> Handling of transmit commands is rather complex,
>>>>>> so about 80 lines of code were moved from function
>>>>>> action_command to the new function tx_command.
>>>>>>
>>>>>> The two new values "tx" and "cb_address" in the
>>>>>> eepro100 status structure made this possible without
>>>>>> passing too many parameters.
>>>>>>
>>>>>> In addition, the moved code was cleaned a little bit:
>>>>>> old comments marked with //~ were removed, C++ style
>>>>>> comments were replaced by C style comments, C++ like
>>>>>> variable declarations after code were reordered.
>>>>>>
>>>>>> Simplified mode is still broken. Nor did I fix
>>>>>> endianess issues. Both problems will be fixed in
>>>>>> additional patches (which need this one).
>>>>>>
>>>>>> Signed-off-by: Stefan Weil <address@hidden>
>>>>>> ---
>>>>>> hw/eepro100.c | 192
>>>>>> +++++++++++++++++++++++++++++----------------------------
>>>>>> 1 files changed, 98 insertions(+), 94 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>>>>> index 4210d8a..7093af8 100644
>>>>>> --- a/hw/eepro100.c
>>>>>> +++ b/hw/eepro100.c
>>>>>> @@ -213,6 +213,10 @@ typedef struct {
>>>>>> uint32_t ru_offset; /* RU address offset */
>>>>>> uint32_t statsaddr; /* pointer to eepro100_stats_t */
>>>>>>
>>>>>> + /* Temporary data. */
>>>>>> + eepro100_tx_t tx;
>>>>>> + uint32_t cb_address;
>>>>>> +
>>>>> That's pretty evil, as it makes routines non-reentrant.
>>>>> How bad is it to pass 2 additional arguments around?
>>>>> If not, maybe define struct tx_command and put necessary stuff there,
>>>>> then pass pointer to that?
>>>> Yes, I know that it makes routines non-reentrant, or
>>>> to be more exact: it makes routines non-reentrant
>>>> for the same device instance. Different device instances
>>>> are reentrant because each instance maintains its
>>>> own status.
>>>>
>>>> No, it's not evil.
>>> "Temporary data" comment is very evil.
>>> We not only don't want to document code,
>>> we document this fact.
>>>
>>>> The state machine of the real hardware
>>>> is not expected to be used in a reentrant way. The same
>>>> applies to the emulated hardware.
>>> That code does not appear currently structured as a state machine.
>>>
>>>> Do you expect reentrant calls of transmit or receive
>>>> functions for one device instance?
>>>>
>>>> Regards,
>>>> Stefan
>>> Datastructures should make sense. This one does not seem to make sense.
>>> What's the benefit here? Why is it good to pass less
>>> parameters to functions?
>> Hello Michael,
>>
>> I don't think that "very evil" is a good way to describe code someone
>> has written.
>> But maybe there is a misunderstanding caused because you and I are not
>> native
>> english speakers.
>>
>> Antony, perhaps a better comment for the two temporary values would be
>> "Temporary data used to pass status information between functions, don't
>> save it."
>
> This does not address the basic problems that 1. just figuring out
> what data should a field contain requires reading code that
> initializes it 2. the code becomes non-reentrant and more fragile.

1. Yes, of course you have to read code to understand it.
   I don't see  your point here - maybe a problem of the language.
2. Reentrancy is not a problem here (I explained that already).
   "More fragile" is a subjective feedback, my feeling is different.


>
>> Feel free to change this comment. Michael, maybe you have a better
>> suggestion?
>
> Yes. Add a couple of extra parameters to the functions that
> you call, with names specifying what do the values contain.

There are many ways to do things in programming.
I decided to do it the way it is, and you prefer a different way.
That's ok, but as long as there are no better arguments,
let me do it my way (up to now, eepro100.c was written
and maintained by me).

>
>> Both values will be used in more functions with patches to come,
>> sometimes also as output parameter. So adding a new status structure for
>> temporary status values or even passing simple data types
>> would make the code less comprehensible.
>>
>> Kind regards,
>> Stefan
>
> I haven't seen that code so I can not judge.

Here is the URL:
http://repo.or.cz/w/qemu/ar7.git/blob/HEAD:/hw/eepro100.c

>
> On the surface, it sounds like using parameters will be better.
> It is a *good idea* to pass input parameters as int X and
> output parameters as int *X, rather than sticking them in some random
> structure just because same function happens to already get a pointer
> to it.

If it were some random structure, you would be right.
I repeat myself: it is not some random structure but the
device status. Both values are device status values.