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

[Anthony Liguori]

Stefan Weil wrote:
> Stefan Weil schrieb:
>   
> > 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(-)
> >     
>
> This patch is still missing in QEMU master.
> Because of other patches (which were sent later!), a rebase is needed again
> (it was rebased 3 times now). Only the line with qemu_send_packet changed.
>
> Please apply the rebased patch if there are no really important
> reasons against it (there are none), hopefully without waiting several
> weeks again.
> It is needed for very important bug fixes (multicast, simple transmit mode).
>   

Michael had given the patch some feedback that I think is valuable.  I 
haven't seen a response to his feedback nor have I seen a resubmit 
incorporating the feedback.

Storing intermediate state of a function as global state (instead of on 
the stack) is odd.  Even if there's a great justification (I don't think 
there is), such a thing is sufficiently unusual that it deserves a 
better comment.

BTW, part of the trouble your having is you're sending one patch that 
does too many things.  If you submitted a series that contained code 
cleanups, and then code motion, the first patches would have been no 
brainers to commit while the code motion would have been treated 
separately making rebasing less painful.

Regards,

Anthony Liguori