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 <w...@mail.berlios.de> >>>>>> --- >>>>>> 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.