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. > 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. > 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. 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. -- MST