From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NJvYE-0004Yl-J2 for qemu-devel@nongnu.org; Sun, 13 Dec 2009 16:00:22 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NJvY8-0004YD-7o for qemu-devel@nongnu.org; Sun, 13 Dec 2009 16:00:20 -0500 Received: from [199.232.76.173] (port=55926 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NJvY8-0004Y5-0I for qemu-devel@nongnu.org; Sun, 13 Dec 2009 16:00:16 -0500 Received: from moutng.kundenserver.de ([212.227.126.186]:64193) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NJvY7-0007l2-An for qemu-devel@nongnu.org; Sun, 13 Dec 2009 16:00:15 -0500 Message-ID: <4B2555CD.3070805@mail.berlios.de> Date: Sun, 13 Dec 2009 21:59:57 +0100 From: Stefan Weil MIME-Version: 1.0 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> <20091213081821.GB1548@redhat.com> In-Reply-To: <20091213081821.GB1548@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" Cc: Anthony Liguori , QEMU Developers 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 >>>>>> --- >>>>>> 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.