From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53052) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QhJS4-0001es-0A for qemu-devel@nongnu.org; Thu, 14 Jul 2011 06:47:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QhJS2-0006Ja-NO for qemu-devel@nongnu.org; Thu, 14 Jul 2011 06:47:27 -0400 Received: from mail-ww0-f53.google.com ([74.125.82.53]:44133) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QhJS2-0006JQ-8f for qemu-devel@nongnu.org; Thu, 14 Jul 2011 06:47:26 -0400 Received: by wwf26 with SMTP id 26so174823wwf.10 for ; Thu, 14 Jul 2011 03:47:25 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <4E1EC93A.2010701@redhat.com> Date: Thu, 14 Jul 2011 12:47:22 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1310637335-15020-1-git-send-email-minovotn@redhat.com> In-Reply-To: <1310637335-15020-1-git-send-email-minovotn@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] Introduce "info migrate-times" monitor command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michal Novotny Cc: qemu-devel@nongnu.org On 07/14/2011 11:55 AM, Michal Novotny wrote: > +/* Time measuring facility */ > +extern int time_measurement_type; > +extern uint64_t time_saveram1; > +extern uint64_t time_saveram2; > +extern uint64_t time_saveram3; > +extern uint64_t time_savedisk1; > +extern uint64_t time_savedisk2; > +extern uint64_t time_savedisk3; > +extern uint64_t time_savetotal1; > +extern uint64_t time_savetotal2; > +extern uint64_t time_savetotal3; Arrays, please. :) Or perhaps you can store this information directly into a QDict. I'm not sure, read until the end. > +extern uint64_t time_savewait_input0; > +extern uint64_t time_save_measured; > +extern uint64_t time_loadpart1; > +extern uint64_t time_loadpart2; > +extern uint64_t time_loadpart3; > +extern uint64_t time_loadpart4; > +extern uint64_t time_load_measured; > + > +#define TIME_MEASUREMENT_NONE 0x00 > +#define TIME_MEASUREMENT_SAVE 0x01 > +#define TIME_MEASUREMENT_LOAD 0x02 > +#define TIME_GET(type, name, stage) time_##type##name##stage > +#define TIME_SET(type, name, stage, tv) time_##type##name##stage = tv > +#define TIME_ADD(type, name, stage, tv) time_##type##name##stage += tv This forces type/name/stage to be fixed, so it is worse than before; at this point you could remove the macros altogether. What I disliked in v1 was: 1) this part (duplicated twice, in time_set and time_add) if (strcmp(name, "ram") == 0) time_set_ram(stage, tv); if (strcmp(name, "disk") == 0) time_set_disk(stage, tv); if (strcmp(name, "wait-input") == 0) time_save_wait_input = tv; if (strcmp(name, "total") == 0) time_set_total(stage, tv); 2) even more shotgun cut-and-paste in the small functions +static void time_set_ram(int stage, uint64_t tv) +{ + if ((stage < 0) || (stage > 3)) + return; + + time_save_ram[stage - 1] = tv; +} + +static void time_set_disk(int stage, uint64_t tv) +{ + if ((stage < 0) || (stage > 3)) + return; + + time_save_disk[stage - 1] = tv; +} + etc. (By the way, the test on stage should be <= 0 for all of them!). 3) the fact that you put this in vl.c Whenever you have duplication like that, you probably should use a more generic data structure, or not use any data structure at all (just variables). In the latter case, no funky macros or nothing---just variables. My first thought was to inline everything, but given how v2 looks like, perhaps the abstraction was actually good to have---just the implementation was ugly---and my judgement was wrong. Perhaps you can store everything from the beginning in the QDict that you will use for the monitor command? Paolo