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

["Michael S. Tsirkin" <mst@redhat.com>]

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 <weil@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?

-- 
MST