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

[Michael S. Tsirkin]

On Sun, Dec 13, 2009 at 09:59:57PM +0100, Stefan Weil wrote:
> 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.

Well, can you please try to document them harder than "Temporary
values"? E.g. write what they represent and when are they valid.

-- 
MST