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

[Stefan Weil]

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."
Feel free to change this comment. Michael, maybe you have a better
suggestion?

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