From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NJjhX-000162-NU for qemu-devel@nongnu.org; Sun, 13 Dec 2009 03:21:11 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NJjhT-00014a-SW for qemu-devel@nongnu.org; Sun, 13 Dec 2009 03:21:11 -0500 Received: from [199.232.76.173] (port=46529 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NJjhT-00014X-Kt for qemu-devel@nongnu.org; Sun, 13 Dec 2009 03:21:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37031) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NJjhT-0005el-CZ for qemu-devel@nongnu.org; Sun, 13 Dec 2009 03:21:07 -0500 Date: Sun, 13 Dec 2009 10:18:21 +0200 From: "Michael S. Tsirkin" Message-ID: <20091213081821.GB1548@redhat.com> References: <1258664086-20264-1-git-send-email-weil@mail.berlios.de> <20091130093015.GA22395@redhat.com> <4B13FB5B.1070109@mail.berlios.de> <20091130203919.GA1496@redhat.com> <4B23F2C2.1010604@mail.berlios.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B23F2C2.1010604@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: Anthony Liguori , QEMU Developers 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 > >>>> --- > >>>> 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