| Message ID | 1355817223-13076-2-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On Tue, 18 Dec 2012 08:53:40 +0100 Andreas Färber <afaerber@suse.de> wrote: > This generalizes {ARM,M68k,Alpha}CPUListState to avoid declaring it for > each target. > > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > include/qemu/cpu.h | 12 ++++++++++++ > target-alpha/cpu.c | 9 ++------- > target-arm/helper.c | 9 ++------- > target-m68k/helper.c | 9 ++------- > 4 Dateien geändert, 18 Zeilen hinzugefügt(+), 21 Zeilen entfernt(-) > Could we use cpus.h for CPUListState? It has related list_cpus() and it doesn't included in any headers yet. And we won't pollute base CPU qom definition with utility structures. > diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h > index 61b7698..5fbb3f9 100644 > --- a/include/qemu/cpu.h > +++ b/include/qemu/cpu.h > @@ -21,6 +21,7 @@ > #define QEMU_CPU_H > > #include "qemu/object.h" > +#include "qemu-common.h" > #include "qemu-thread.h" > > /** > @@ -80,6 +81,17 @@ struct CPUState { > /* TODO Move common fields from CPUArchState here. */ > }; > > +/** > + * CPUListState: > + * @cpu_fprintf: Print function. > + * @file: File to print to using @cpu_fprint. > + * > + * State commonly used for iterating over CPU models. > + */ > +typedef struct CPUListState { > + fprintf_function cpu_fprintf; > + FILE *file; > +} CPUListState; > > /** > * cpu_reset: > diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c > index d065085..915278f 100644 > --- a/target-alpha/cpu.c > +++ b/target-alpha/cpu.c > @@ -33,11 +33,6 @@ static void alpha_cpu_realize(Object *obj, Error **err) > #endif > } > > -typedef struct AlphaCPUListState { > - fprintf_function cpu_fprintf; > - FILE *file; > -} AlphaCPUListState; > - > /* Sort alphabetically by type name. */ > static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b) > { > @@ -53,7 +48,7 @@ static gint alpha_cpu_list_compare(gconstpointer a, > gconstpointer b) static void alpha_cpu_list_entry(gpointer data, gpointer Perhaps *cpu_list_entry() could be generalized too, they all look alike. > user_data) { > ObjectClass *oc = data; > - AlphaCPUListState *s = user_data; > + CPUListState *s = user_data; > > (*s->cpu_fprintf)(s->file, " %s\n", > object_class_get_name(oc)); > @@ -61,7 +56,7 @@ static void alpha_cpu_list_entry(gpointer data, gpointer > user_data) > void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf) > { > - AlphaCPUListState s = { > + CPUListState s = { > .file = f, > .cpu_fprintf = cpu_fprintf, > }; > diff --git a/target-arm/helper.c b/target-arm/helper.c > index ab8b734..d2f2fb4 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -1291,11 +1291,6 @@ ARMCPU *cpu_arm_init(const char *cpu_model) > return cpu; > } > > -typedef struct ARMCPUListState { > - fprintf_function cpu_fprintf; > - FILE *file; > -} ARMCPUListState; > - > /* Sort alphabetically by type name, except for "any". */ > static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b) > { > @@ -1317,7 +1312,7 @@ static gint arm_cpu_list_compare(gconstpointer a, > gconstpointer b) static void arm_cpu_list_entry(gpointer data, gpointer > user_data) { > ObjectClass *oc = data; > - ARMCPUListState *s = user_data; > + CPUListState *s = user_data; > > (*s->cpu_fprintf)(s->file, " %s\n", > object_class_get_name(oc)); > @@ -1325,7 +1320,7 @@ static void arm_cpu_list_entry(gpointer data, > gpointer user_data) > void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf) > { > - ARMCPUListState s = { > + CPUListState s = { > .file = f, > .cpu_fprintf = cpu_fprintf, > }; > diff --git a/target-m68k/helper.c b/target-m68k/helper.c > index a5d0100..875a71a 100644 > --- a/target-m68k/helper.c > +++ b/target-m68k/helper.c > @@ -25,11 +25,6 @@ > > #define SIGNBIT (1u << 31) > > -typedef struct M68kCPUListState { > - fprintf_function cpu_fprintf; > - FILE *file; > -} M68kCPUListState; > - > /* Sort alphabetically, except for "any". */ > static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b) > { > @@ -51,7 +46,7 @@ static gint m68k_cpu_list_compare(gconstpointer a, > gconstpointer b) static void m68k_cpu_list_entry(gpointer data, gpointer > user_data) { > ObjectClass *c = data; > - M68kCPUListState *s = user_data; > + CPUListState *s = user_data; > > (*s->cpu_fprintf)(s->file, "%s\n", > object_class_get_name(c)); > @@ -59,7 +54,7 @@ static void m68k_cpu_list_entry(gpointer data, gpointer > user_data) > void m68k_cpu_list(FILE *f, fprintf_function cpu_fprintf) > { > - M68kCPUListState s = { > + CPUListState s = { > .file = f, > .cpu_fprintf = cpu_fprintf, > };
On Tue, Dec 18, 2012 at 08:53:40AM +0100, Andreas Färber wrote: > This generalizes {ARM,M68k,Alpha}CPUListState to avoid declaring it for > each target. > > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > include/qemu/cpu.h | 12 ++++++++++++ > target-alpha/cpu.c | 9 ++------- > target-arm/helper.c | 9 ++------- > target-m68k/helper.c | 9 ++------- > 4 Dateien geändert, 18 Zeilen hinzugefügt(+), 21 Zeilen entfernt(-) > > diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h > index 61b7698..5fbb3f9 100644 > --- a/include/qemu/cpu.h > +++ b/include/qemu/cpu.h > @@ -21,6 +21,7 @@ > #define QEMU_CPU_H > > #include "qemu/object.h" > +#include "qemu-common.h" > #include "qemu-thread.h" Please, don't add more "#include qemu-common.h" lines to header files. This introduces yet another circular dependency: qemu-common.h -> target-*/cpu.h -> target-*/cpu-qom.h -> qemu/cpu.h -> qemu-common.h You could just reverse the order of patches 1/4 and 2/4, and include "qemu-types.h" instead. The rest of the patch is an obvious removal of duplicate code, that would get a Reviewed-By line from me. > > /** > @@ -80,6 +81,17 @@ struct CPUState { > /* TODO Move common fields from CPUArchState here. */ > }; > > +/** > + * CPUListState: > + * @cpu_fprintf: Print function. > + * @file: File to print to using @cpu_fprint. > + * > + * State commonly used for iterating over CPU models. > + */ > +typedef struct CPUListState { > + fprintf_function cpu_fprintf; > + FILE *file; > +} CPUListState; > > /** > * cpu_reset: > diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c > index d065085..915278f 100644 > --- a/target-alpha/cpu.c > +++ b/target-alpha/cpu.c > @@ -33,11 +33,6 @@ static void alpha_cpu_realize(Object *obj, Error **err) > #endif > } > > -typedef struct AlphaCPUListState { > - fprintf_function cpu_fprintf; > - FILE *file; > -} AlphaCPUListState; > - > /* Sort alphabetically by type name. */ > static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b) > { > @@ -53,7 +48,7 @@ static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b) > static void alpha_cpu_list_entry(gpointer data, gpointer user_data) > { > ObjectClass *oc = data; > - AlphaCPUListState *s = user_data; > + CPUListState *s = user_data; > > (*s->cpu_fprintf)(s->file, " %s\n", > object_class_get_name(oc)); > @@ -61,7 +56,7 @@ static void alpha_cpu_list_entry(gpointer data, gpointer user_data) > > void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf) > { > - AlphaCPUListState s = { > + CPUListState s = { > .file = f, > .cpu_fprintf = cpu_fprintf, > }; > diff --git a/target-arm/helper.c b/target-arm/helper.c > index ab8b734..d2f2fb4 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -1291,11 +1291,6 @@ ARMCPU *cpu_arm_init(const char *cpu_model) > return cpu; > } > > -typedef struct ARMCPUListState { > - fprintf_function cpu_fprintf; > - FILE *file; > -} ARMCPUListState; > - > /* Sort alphabetically by type name, except for "any". */ > static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b) > { > @@ -1317,7 +1312,7 @@ static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b) > static void arm_cpu_list_entry(gpointer data, gpointer user_data) > { > ObjectClass *oc = data; > - ARMCPUListState *s = user_data; > + CPUListState *s = user_data; > > (*s->cpu_fprintf)(s->file, " %s\n", > object_class_get_name(oc)); > @@ -1325,7 +1320,7 @@ static void arm_cpu_list_entry(gpointer data, gpointer user_data) > > void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf) > { > - ARMCPUListState s = { > + CPUListState s = { > .file = f, > .cpu_fprintf = cpu_fprintf, > }; > diff --git a/target-m68k/helper.c b/target-m68k/helper.c > index a5d0100..875a71a 100644 > --- a/target-m68k/helper.c > +++ b/target-m68k/helper.c > @@ -25,11 +25,6 @@ > > #define SIGNBIT (1u << 31) > > -typedef struct M68kCPUListState { > - fprintf_function cpu_fprintf; > - FILE *file; > -} M68kCPUListState; > - > /* Sort alphabetically, except for "any". */ > static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b) > { > @@ -51,7 +46,7 @@ static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b) > static void m68k_cpu_list_entry(gpointer data, gpointer user_data) > { > ObjectClass *c = data; > - M68kCPUListState *s = user_data; > + CPUListState *s = user_data; > > (*s->cpu_fprintf)(s->file, "%s\n", > object_class_get_name(c)); > @@ -59,7 +54,7 @@ static void m68k_cpu_list_entry(gpointer data, gpointer user_data) > > void m68k_cpu_list(FILE *f, fprintf_function cpu_fprintf) > { > - M68kCPUListState s = { > + CPUListState s = { > .file = f, > .cpu_fprintf = cpu_fprintf, > }; > -- > 1.7.10.4 > >
Am 18.12.2012 16:59, schrieb Igor Mammedov: > On Tue, 18 Dec 2012 08:53:40 +0100 > Andreas Färber <afaerber@suse.de> wrote: > >> This generalizes {ARM,M68k,Alpha}CPUListState to avoid declaring it for >> each target. >> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> --- >> include/qemu/cpu.h | 12 ++++++++++++ >> target-alpha/cpu.c | 9 ++------- >> target-arm/helper.c | 9 ++------- >> target-m68k/helper.c | 9 ++------- >> 4 Dateien geändert, 18 Zeilen hinzugefügt(+), 21 Zeilen entfernt(-) >> > > Could we use cpus.h for CPUListState? > > It has related list_cpus() and it doesn't included in any headers yet. > And we won't pollute base CPU qom definition with utility structures. I'd be open for another header, but it's probably orthogonal to the qemu-common.h dependency issue. Note however that I have been moving the QOM-cleaned-up declarations of CPU functions to qemu/cpu.h as the canonical CPU header as well. CPUListState does not contain CPUState in any way, it's more for the target than for the actual CPU, so that's a good point to investigate. >> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c >> index d065085..915278f 100644 >> --- a/target-alpha/cpu.c >> +++ b/target-alpha/cpu.c >> @@ -33,11 +33,6 @@ static void alpha_cpu_realize(Object *obj, Error **err) >> #endif >> } >> >> -typedef struct AlphaCPUListState { >> - fprintf_function cpu_fprintf; >> - FILE *file; >> -} AlphaCPUListState; >> - >> /* Sort alphabetically by type name. */ >> static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b) >> { >> @@ -53,7 +48,7 @@ static gint alpha_cpu_list_compare(gconstpointer a, >> gconstpointer b) static void alpha_cpu_list_entry(gpointer data, gpointer > Perhaps *cpu_list_entry() could be generalized too, they all look alike. Actually, apart from varying indentation and whether or not they're prefixed with the architecture, some print more information than just the class name. In particular other targets print just the base name for backwards compatibility (something that I could optionally change for alpha, where we don't have any to keep). What I was considering was to move to a general location a GCompareFunc comparing just the ObjectClass names. But so far there's always exceptions to that rule (any, host, PVR order, new vs. old naming scheme, ...) so that no two are identical yet. Andreas
Am 18.12.2012 18:42, schrieb Eduardo Habkost: > On Tue, Dec 18, 2012 at 08:53:40AM +0100, Andreas Färber wrote: >> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h >> index 61b7698..5fbb3f9 100644 >> --- a/include/qemu/cpu.h >> +++ b/include/qemu/cpu.h >> @@ -21,6 +21,7 @@ >> #define QEMU_CPU_H >> >> #include "qemu/object.h" >> +#include "qemu-common.h" >> #include "qemu-thread.h" > > Please, don't add more "#include qemu-common.h" lines to header files. > This introduces yet another circular dependency: > > qemu-common.h -> target-*/cpu.h -> target-*/cpu-qom.h -> qemu/cpu.h -> qemu-common.h That's what 2/4 resolves. My reasoning was that this should be an uncontroversial code-sharing change since, for good or bad, qemu-common.h happens to be the place where this is defined today. Whether to move it to qemu-types.h as proposed or to a new qemu-stdio.h affects more than just the core CPU and thus me as maintainer and requires careful mingw32 etc. testing. > You could just reverse the order of patches 1/4 and 2/4, and include > "qemu-types.h" instead. If we find an agreeable solution by tomorrow for how/where to do it, sure! Andreas > The rest of the patch is an obvious removal of duplicate code, that > would get a Reviewed-By line from me.
diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h index 61b7698..5fbb3f9 100644 --- a/include/qemu/cpu.h +++ b/include/qemu/cpu.h @@ -21,6 +21,7 @@ #define QEMU_CPU_H #include "qemu/object.h" +#include "qemu-common.h" #include "qemu-thread.h" /** @@ -80,6 +81,17 @@ struct CPUState { /* TODO Move common fields from CPUArchState here. */ }; +/** + * CPUListState: + * @cpu_fprintf: Print function. + * @file: File to print to using @cpu_fprint. + * + * State commonly used for iterating over CPU models. + */ +typedef struct CPUListState { + fprintf_function cpu_fprintf; + FILE *file; +} CPUListState; /** * cpu_reset: diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c index d065085..915278f 100644 --- a/target-alpha/cpu.c +++ b/target-alpha/cpu.c @@ -33,11 +33,6 @@ static void alpha_cpu_realize(Object *obj, Error **err) #endif } -typedef struct AlphaCPUListState { - fprintf_function cpu_fprintf; - FILE *file; -} AlphaCPUListState; - /* Sort alphabetically by type name. */ static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b) { @@ -53,7 +48,7 @@ static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b) static void alpha_cpu_list_entry(gpointer data, gpointer user_data) { ObjectClass *oc = data; - AlphaCPUListState *s = user_data; + CPUListState *s = user_data; (*s->cpu_fprintf)(s->file, " %s\n", object_class_get_name(oc)); @@ -61,7 +56,7 @@ static void alpha_cpu_list_entry(gpointer data, gpointer user_data) void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf) { - AlphaCPUListState s = { + CPUListState s = { .file = f, .cpu_fprintf = cpu_fprintf, }; diff --git a/target-arm/helper.c b/target-arm/helper.c index ab8b734..d2f2fb4 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -1291,11 +1291,6 @@ ARMCPU *cpu_arm_init(const char *cpu_model) return cpu; } -typedef struct ARMCPUListState { - fprintf_function cpu_fprintf; - FILE *file; -} ARMCPUListState; - /* Sort alphabetically by type name, except for "any". */ static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b) { @@ -1317,7 +1312,7 @@ static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b) static void arm_cpu_list_entry(gpointer data, gpointer user_data) { ObjectClass *oc = data; - ARMCPUListState *s = user_data; + CPUListState *s = user_data; (*s->cpu_fprintf)(s->file, " %s\n", object_class_get_name(oc)); @@ -1325,7 +1320,7 @@ static void arm_cpu_list_entry(gpointer data, gpointer user_data) void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf) { - ARMCPUListState s = { + CPUListState s = { .file = f, .cpu_fprintf = cpu_fprintf, }; diff --git a/target-m68k/helper.c b/target-m68k/helper.c index a5d0100..875a71a 100644 --- a/target-m68k/helper.c +++ b/target-m68k/helper.c @@ -25,11 +25,6 @@ #define SIGNBIT (1u << 31) -typedef struct M68kCPUListState { - fprintf_function cpu_fprintf; - FILE *file; -} M68kCPUListState; - /* Sort alphabetically, except for "any". */ static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b) { @@ -51,7 +46,7 @@ static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b) static void m68k_cpu_list_entry(gpointer data, gpointer user_data) { ObjectClass *c = data; - M68kCPUListState *s = user_data; + CPUListState *s = user_data; (*s->cpu_fprintf)(s->file, "%s\n", object_class_get_name(c)); @@ -59,7 +54,7 @@ static void m68k_cpu_list_entry(gpointer data, gpointer user_data) void m68k_cpu_list(FILE *f, fprintf_function cpu_fprintf) { - M68kCPUListState s = { + CPUListState s = { .file = f, .cpu_fprintf = cpu_fprintf, };
This generalizes {ARM,M68k,Alpha}CPUListState to avoid declaring it for each target. Signed-off-by: Andreas Färber <afaerber@suse.de> --- include/qemu/cpu.h | 12 ++++++++++++ target-alpha/cpu.c | 9 ++------- target-arm/helper.c | 9 ++------- target-m68k/helper.c | 9 ++------- 4 Dateien geändert, 18 Zeilen hinzugefügt(+), 21 Zeilen entfernt(-)