Patchwork [5/7] add qdev property type "cpu"

login
register
mail settings
Submitter Paolo Bonzini
Date June 25, 2010, 12:52 p.m.
Message ID <1277470342-5861-6-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/56973/
State New
Headers show

Comments

Paolo Bonzini - June 25, 2010, 12:52 p.m.
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(-)
Markus Armbruster - June 26, 2010, 7:46 a.m.
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.

[...]
Paolo Bonzini - June 27, 2010, 12:48 p.m.
> 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
Blue Swirl - June 27, 2010, 6:45 p.m.
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
>
>
>
>
Markus Armbruster - June 29, 2010, 11:42 a.m.
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.
Paolo Bonzini - June 29, 2010, 1:52 p.m.
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

Patch

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);