| Message ID | 1277470342-5861-6-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Paolo Bonzini <pbonzini@redhat.com> writes: [...] > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c > index 5a8739d..2759c83 100644 > --- a/hw/qdev-properties.c > +++ b/hw/qdev-properties.c > @@ -1,6 +1,7 @@ > #include "net.h" > #include "qdev.h" > #include "qerror.h" > +#include "cpus.h" > > void *qdev_get_prop_ptr(DeviceState *dev, Property *prop) > { > @@ -281,6 +282,44 @@ PropertyInfo qdev_prop_string = { > .free = free_string, > }; > > +/* --- cpu --- */ > + > +static int parse_cpu(DeviceState *dev, Property *prop, const char *str) > +{ > + CPUState **ptr = qdev_get_prop_ptr(dev, prop); > + char *end; > + int id; > + > + if (!*str) > + return -ENOENT; > + > + id = strtol (str, &end, 0); > + if (*end) > + return -ENOENT; > + > + *ptr = cpu_get_by_id(id); > + if (*ptr == NULL) > + return -ENOENT; > + return 0; > +} > + > +static int print_cpu(DeviceState *dev, Property *prop, char *dest, size_t len) > +{ > + CPUState **ptr = qdev_get_prop_ptr(dev, prop); > + if (*ptr) > + return snprintf(dest, len, "CPU #%d", cpu_get_id(*ptr)); > + else > + return snprintf(dest, len, "CPU #<null>"); > +} Hmm. Parse method doesn't accept output of the print method. Not so nice. Is the "CPU #" decoration essential? Oh, and beware of the Secret Tab Police. [...]
> Hmm. Parse method doesn't accept output of the print method. Not so > nice. Is the "CPU #" decoration essential? I noticed the same in parse/print string: static int parse_string(DeviceState *dev, Property *prop, const char *str) { char **ptr = qdev_get_prop_ptr(dev, prop); if (*ptr) qemu_free(*ptr); *ptr = qemu_strdup(str); return 0; } static int print_string(DeviceState *dev, Property *prop, char *dest, size_t len) { char **ptr = qdev_get_prop_ptr(dev, prop); if (!*ptr) return snprintf(dest, len, "<null>"); return snprintf(dest, len, "\"%s\"", *ptr); } It looks like printing representation is chosen "for the user", not for parsing. Paolo
On Fri, Jun 25, 2010 at 12:52 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > cpus.c | 16 ++++++++++++++++ > cpus.h | 2 ++ > hw/qdev-properties.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > hw/qdev.h | 5 +++++ > 4 files changed, 67 insertions(+), 0 deletions(-) > > diff --git a/cpus.c b/cpus.c > index fcd0f09..da6ec44 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -91,6 +91,22 @@ void cpu_synchronize_all_post_init(void) > } > } > > +CPUState *cpu_get_by_id(int id) > +{ > + CPUState *cpu; > + > + for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) > + if (cpu->cpu_index == id) > + return cpu; CODING_STYLE, also below. > + > + return NULL; > +} > + > +int cpu_get_id(CPUState *env) > +{ > + return env->cpu_index; > +} > + > int cpu_is_stopped(CPUState *env) > { > return !vm_running || env->stopped; > diff --git a/cpus.h b/cpus.h > index 774150a..df3c193 100644 > --- a/cpus.h > +++ b/cpus.h > @@ -6,6 +6,8 @@ int qemu_init_main_loop(void); > void qemu_main_loop_start(void); > void resume_all_vcpus(void); > void pause_all_vcpus(void); > +CPUState *cpu_get_by_id(int id); > +int cpu_get_id(CPUState *env); > > /* vl.c */ > extern int smp_cores; > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c > index 5a8739d..2759c83 100644 > --- a/hw/qdev-properties.c > +++ b/hw/qdev-properties.c > @@ -1,6 +1,7 @@ > #include "net.h" > #include "qdev.h" > #include "qerror.h" > +#include "cpus.h" > > void *qdev_get_prop_ptr(DeviceState *dev, Property *prop) > { > @@ -281,6 +282,44 @@ PropertyInfo qdev_prop_string = { > .free = free_string, > }; > > +/* --- cpu --- */ > + > +static int parse_cpu(DeviceState *dev, Property *prop, const char *str) > +{ > + CPUState **ptr = qdev_get_prop_ptr(dev, prop); > + char *end; > + int id; > + > + if (!*str) > + return -ENOENT; > + > + id = strtol (str, &end, 0); > + if (*end) > + return -ENOENT; > + > + *ptr = cpu_get_by_id(id); > + if (*ptr == NULL) > + return -ENOENT; > + return 0; > +} > + > +static int print_cpu(DeviceState *dev, Property *prop, char *dest, size_t len) > +{ > + CPUState **ptr = qdev_get_prop_ptr(dev, prop); > + if (*ptr) > + return snprintf(dest, len, "CPU #%d", cpu_get_id(*ptr)); > + else > + return snprintf(dest, len, "CPU #<null>"); > +} > + > +PropertyInfo qdev_prop_cpu = { > + .name = "cpu", > + .type = PROP_TYPE_CPU, > + .size = sizeof(DriveInfo*), > + .parse = parse_cpu, > + .print = print_cpu, > +}; > + > /* --- drive --- */ > > static int parse_drive(DeviceState *dev, Property *prop, const char *str) > @@ -657,6 +696,11 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value) > qdev_prop_set(dev, name, &value, PROP_TYPE_PTR); > } > > +void qdev_prop_set_cpu(DeviceState *dev, const char *name, CPUState *value) > +{ > + qdev_prop_set(dev, name, &value, PROP_TYPE_CPU); > +} > + > void qdev_prop_set_defaults(DeviceState *dev, Property *props) > { > if (!props) > diff --git a/hw/qdev.h b/hw/qdev.h > index be5ad67..eec2f52 100644 > --- a/hw/qdev.h > +++ b/hw/qdev.h > @@ -90,6 +90,7 @@ enum PropertyType { > PROP_TYPE_VLAN, > PROP_TYPE_PTR, > PROP_TYPE_BIT, > + PROP_TYPE_CPU, > }; > > struct PropertyInfo { > @@ -203,6 +204,7 @@ extern PropertyInfo qdev_prop_drive; > extern PropertyInfo qdev_prop_netdev; > extern PropertyInfo qdev_prop_vlan; > extern PropertyInfo qdev_prop_pci_devfn; > +extern PropertyInfo qdev_prop_cpu; > > #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \ > .name = (_name), \ > @@ -257,6 +259,8 @@ extern PropertyInfo qdev_prop_pci_devfn; > DEFINE_PROP(_n, _s, _f, qdev_prop_drive, DriveInfo*) > #define DEFINE_PROP_MACADDR(_n, _s, _f) \ > DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr) > +#define DEFINE_PROP_CPU(_n, _s, _f) \ > + DEFINE_PROP(_n, _s, _f, qdev_prop_cpu, CPUState*) > > #define DEFINE_PROP_END_OF_LIST() \ > {} > @@ -276,6 +280,7 @@ void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *valu > void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value); > void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value); > void qdev_prop_set_drive(DeviceState *dev, const char *name, DriveInfo *value); > +void qdev_prop_set_cpu(DeviceState *dev, const char *name, CPUState *value); > void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value); > /* FIXME: Remove opaque pointer properties. */ > void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value); > -- > 1.7.0.1 > > > >
Paolo Bonzini <pbonzini@redhat.com> writes: >> Hmm. Parse method doesn't accept output of the print method. Not so >> nice. Is the "CPU #" decoration essential? > > I noticed the same in parse/print string: > > static int parse_string(DeviceState *dev, Property *prop, const char *str) > { > char **ptr = qdev_get_prop_ptr(dev, prop); > > if (*ptr) > qemu_free(*ptr); > *ptr = qemu_strdup(str); > return 0; > } > > static int print_string(DeviceState *dev, Property *prop, char *dest, > size_t len) > { > char **ptr = qdev_get_prop_ptr(dev, prop); > if (!*ptr) > return snprintf(dest, len, "<null>"); > return snprintf(dest, len, "\"%s\"", *ptr); > } > > It looks like printing representation is chosen "for the user", not > for parsing. Yes, but does that mean we *should* add such decorations? For me, the print method should be reasonably close to what the parse method accepts. A bit like Lisp's princ, whose ouput is close, but not identical to what the reader accepts (for output acceptable to the reader, use prin1). As to "for the user": dev-prop: cpu_env = CPU #0 dev-prop: cpu_env = 0 Both are equally intelligible, in my opinion: obvious if you know what the property is about, gibberish if you don't. The latter is slightly easier to parse with simple tools like AWK. By the way, print_string() should escape double-quote and funny characters.
On 06/29/2010 01:42 PM, Markus Armbruster wrote: > As to "for the user": > > dev-prop: cpu_env = CPU #0 > dev-prop: cpu_env = 0 > > Both are equally intelligible, in my opinion: obvious if you know what > the property is about, gibberish if you don't. The latter is slightly > easier to parse with simple tools like AWK. I'll remove the decoration (though the overall problem remains). Paolo
diff --git a/cpus.c b/cpus.c index fcd0f09..da6ec44 100644 --- a/cpus.c +++ b/cpus.c @@ -91,6 +91,22 @@ void cpu_synchronize_all_post_init(void) } } +CPUState *cpu_get_by_id(int id) +{ + CPUState *cpu; + + for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) + if (cpu->cpu_index == id) + return cpu; + + return NULL; +} + +int cpu_get_id(CPUState *env) +{ + return env->cpu_index; +} + int cpu_is_stopped(CPUState *env) { return !vm_running || env->stopped; diff --git a/cpus.h b/cpus.h index 774150a..df3c193 100644 --- a/cpus.h +++ b/cpus.h @@ -6,6 +6,8 @@ int qemu_init_main_loop(void); void qemu_main_loop_start(void); void resume_all_vcpus(void); void pause_all_vcpus(void); +CPUState *cpu_get_by_id(int id); +int cpu_get_id(CPUState *env); /* vl.c */ extern int smp_cores; diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index 5a8739d..2759c83 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -1,6 +1,7 @@ #include "net.h" #include "qdev.h" #include "qerror.h" +#include "cpus.h" void *qdev_get_prop_ptr(DeviceState *dev, Property *prop) { @@ -281,6 +282,44 @@ PropertyInfo qdev_prop_string = { .free = free_string, }; +/* --- cpu --- */ + +static int parse_cpu(DeviceState *dev, Property *prop, const char *str) +{ + CPUState **ptr = qdev_get_prop_ptr(dev, prop); + char *end; + int id; + + if (!*str) + return -ENOENT; + + id = strtol (str, &end, 0); + if (*end) + return -ENOENT; + + *ptr = cpu_get_by_id(id); + if (*ptr == NULL) + return -ENOENT; + return 0; +} + +static int print_cpu(DeviceState *dev, Property *prop, char *dest, size_t len) +{ + CPUState **ptr = qdev_get_prop_ptr(dev, prop); + if (*ptr) + return snprintf(dest, len, "CPU #%d", cpu_get_id(*ptr)); + else + return snprintf(dest, len, "CPU #<null>"); +} + +PropertyInfo qdev_prop_cpu = { + .name = "cpu", + .type = PROP_TYPE_CPU, + .size = sizeof(DriveInfo*), + .parse = parse_cpu, + .print = print_cpu, +}; + /* --- drive --- */ static int parse_drive(DeviceState *dev, Property *prop, const char *str) @@ -657,6 +696,11 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value) qdev_prop_set(dev, name, &value, PROP_TYPE_PTR); } +void qdev_prop_set_cpu(DeviceState *dev, const char *name, CPUState *value) +{ + qdev_prop_set(dev, name, &value, PROP_TYPE_CPU); +} + void qdev_prop_set_defaults(DeviceState *dev, Property *props) { if (!props) diff --git a/hw/qdev.h b/hw/qdev.h index be5ad67..eec2f52 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -90,6 +90,7 @@ enum PropertyType { PROP_TYPE_VLAN, PROP_TYPE_PTR, PROP_TYPE_BIT, + PROP_TYPE_CPU, }; struct PropertyInfo { @@ -203,6 +204,7 @@ extern PropertyInfo qdev_prop_drive; extern PropertyInfo qdev_prop_netdev; extern PropertyInfo qdev_prop_vlan; extern PropertyInfo qdev_prop_pci_devfn; +extern PropertyInfo qdev_prop_cpu; #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \ .name = (_name), \ @@ -257,6 +259,8 @@ extern PropertyInfo qdev_prop_pci_devfn; DEFINE_PROP(_n, _s, _f, qdev_prop_drive, DriveInfo*) #define DEFINE_PROP_MACADDR(_n, _s, _f) \ DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr) +#define DEFINE_PROP_CPU(_n, _s, _f) \ + DEFINE_PROP(_n, _s, _f, qdev_prop_cpu, CPUState*) #define DEFINE_PROP_END_OF_LIST() \ {} @@ -276,6 +280,7 @@ void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *valu void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value); void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value); void qdev_prop_set_drive(DeviceState *dev, const char *name, DriveInfo *value); +void qdev_prop_set_cpu(DeviceState *dev, const char *name, CPUState *value); void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value); /* FIXME: Remove opaque pointer properties. */ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- cpus.c | 16 ++++++++++++++++ cpus.h | 2 ++ hw/qdev-properties.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ hw/qdev.h | 5 +++++ 4 files changed, 67 insertions(+), 0 deletions(-)