From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NFD4S-0006Ah-Ny for qemu-devel@nongnu.org; Mon, 30 Nov 2009 15:42:08 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NFD4O-000690-03 for qemu-devel@nongnu.org; Mon, 30 Nov 2009 15:42:08 -0500 Received: from [199.232.76.173] (port=60154 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NFD4N-00068r-RJ for qemu-devel@nongnu.org; Mon, 30 Nov 2009 15:42:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:21907) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NFD4N-0001cU-EC for qemu-devel@nongnu.org; Mon, 30 Nov 2009 15:42:03 -0500 Date: Mon, 30 Nov 2009 22:39:20 +0200 From: "Michael S. Tsirkin" Message-ID: <20091130203919.GA1496@redhat.com> References: <1258664086-20264-1-git-send-email-weil@mail.berlios.de> <20091130093015.GA22395@redhat.com> <4B13FB5B.1070109@mail.berlios.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B13FB5B.1070109@mail.berlios.de> Subject: [Qemu-devel] Re: [PATCH] eepro100: Restructure code (new function tx_command) List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: QEMU Developers 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 > >> --- > >> 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