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

[Michael S. Tsirkin]

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.

> 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