Re: [Qemu-devel] [PATCH v2] Introduce "info migrate-times" monitor command

[Paolo Bonzini]

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