[Qemu-devel] [RFC v0] HACK: qom: object_property_set: abort on failure

[Qemu-devel] [RFC v0] HACK: qom: object_property_set: abort on failure

[Peter A. G. Crosthwaite]

Hi All. A couple of times now ive had debug issues due to silent failure of
object_property_set. This function silently fails if the requested property
does not exist for the target object. To trap this error I applied the patch
below to my tree, but I am assuming that this is not mergeable as is as im
guessing there are clients out there that are speculatively trying to set props.

Could someone confirm the expected policy here? is setting a non-existant
property supposed to be a no-op (as it currently is) or should it fail
gracefully?

Whats the best meachinism for creating a no_fail version of object_property_set,
for the 90% case where a non-existant property is an error in machine model
development?

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 qom/object.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index a552be2..6e875a8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -687,7 +687,7 @@ void object_property_set(Object *obj, Visitor *v, const char *name,
 {
     ObjectProperty *prop = object_property_find(obj, name, errp);
     if (prop == NULL) {
-        return;
+        abort();
     }
 
     if (!prop->set) {
-- 
1.7.0.4

Re: [Qemu-devel] [RFC v0] HACK: qom: object_property_set: abort on failure

[Anthony Liguori]

"Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com> writes:

> Hi All. A couple of times now ive had debug issues due to silent failure of
> object_property_set. This function silently fails if the requested property
> does not exist for the target object. To trap this error I applied the patch
> below to my tree, but I am assuming that this is not mergeable as is as im
> guessing there are clients out there that are speculatively trying to set props.
>
> Could someone confirm the expected policy here? is setting a non-existant
> property supposed to be a no-op (as it currently is) or should it fail
> gracefully?

Are you calling set via object_property_set_[str,int,bool,...]?

Why don't you add a terminal error to those functions if setting fails and errp
is NULL.

Regards,

Anthony Liguori

>
> Whats the best meachinism for creating a no_fail version of object_property_set,
> for the 90% case where a non-existant property is an error in machine model
> development?
>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
>  qom/object.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index a552be2..6e875a8 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -687,7 +687,7 @@ void object_property_set(Object *obj, Visitor *v, const char *name,
>  {
>      ObjectProperty *prop = object_property_find(obj, name, errp);
>      if (prop == NULL) {
> -        return;
> +        abort();
>      }
>  
>      if (!prop->set) {
> -- 
> 1.7.0.4

Re: [Qemu-devel] [RFC v0] HACK: qom: object_property_set: abort on failure

[Peter Crosthwaite]

On Tue, Aug 14, 2012 at 10:50 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> "Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com> writes:
>
>> Hi All. A couple of times now ive had debug issues due to silent failure of
>> object_property_set. This function silently fails if the requested property
>> does not exist for the target object. To trap this error I applied the patch
>> below to my tree, but I am assuming that this is not mergeable as is as im
>> guessing there are clients out there that are speculatively trying to set props.
>>
>> Could someone confirm the expected policy here? is setting a non-existant
>> property supposed to be a no-op (as it currently is) or should it fail
>> gracefully?
>
> Are you calling set via object_property_set_[str,int,bool,...]?

object_propert_set_link()
>
> Why don't you add a terminal error to those functions if setting fails and errp
> is NULL.
>

That all works. Thanks for the help.

Do we want no consider adding no_fail() versions of these functions
though? It could get tedious is machine models have to have an
assertion call after every object property set just to check the
property exists. Here's my code as it stands:

Error *errp = NULL;
object_property_set_link(OBJECT(dev), OBJECT(cpus[0]), "cpu0", &errp);
assert_no_error(errp);

Regards,
Peter

> Regards,
>
> Anthony Liguori
>
>>
>> Whats the best meachinism for creating a no_fail version of object_property_set,
>> for the 90% case where a non-existant property is an error in machine model
>> development?
>>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> ---
>>  qom/object.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index a552be2..6e875a8 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -687,7 +687,7 @@ void object_property_set(Object *obj, Visitor *v, const char *name,
>>  {
>>      ObjectProperty *prop = object_property_find(obj, name, errp);
>>      if (prop == NULL) {
>> -        return;
>> +        abort();
>>      }
>>
>>      if (!prop->set) {
>> --
>> 1.7.0.4
>

Re: [Qemu-devel] [RFC v0] HACK: qom: object_property_set: abort on failure

[Andreas Färber]

Am 20.08.2012 04:18, schrieb Peter Crosthwaite:
> [...] Here's my code as it stands:
> 
> Error *errp = NULL;
> object_property_set_link(OBJECT(dev), OBJECT(cpus[0]), "cpu0", &errp);
> assert_no_error(errp);

There's two pitfalls there, the needed object_property_add_link() that
you stumbled over and also the need for a canonical path at the time of
setting the link. What target is this (microblaze?) and which path and
place for adding it as a child do you plan to use?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC v0] HACK: qom: object_property_set: abort on failure

[Peter Crosthwaite]

On Mon, Aug 20, 2012 at 9:02 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 20.08.2012 04:18, schrieb Peter Crosthwaite:
>> [...] Here's my code as it stands:
>>
>> Error *errp = NULL;
>> object_property_set_link(OBJECT(dev), OBJECT(cpus[0]), "cpu0", &errp);
>> assert_no_error(errp);
>
> There's two pitfalls there, the needed object_property_add_link() that
> you stumbled over and also the need for a canonical path at the time of
> setting the link. What target is this (microblaze?) and which path and
> place for adding it as a child do you plan to use?
>

Hi Andreas,

I am aware of the canonical path issue and hacked past it.

This is for ARM zynq - heres my hack in xilinx_zynq.c, that fixes the
canon path issue. This occurs before the object_property_set above().

@@ -50,7 +76,7 @@ static void zynq_init(ram_addr_t ram_size, const
char *boot_device,
                         const char *kernel_filename, const char
*kernel_cmdline,
                         const char *initrd_filename, const char *cpu_model)
 {
-    ARMCPU *cpu;
+    ARMCPU *cpus[2];
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
@@ -60,19 +86,26 @@ static void zynq_init(ram_addr_t ram_size, const
char *boot_device,
     qemu_irq pic[64];
     NICInfo *nd;
     int n;
-    qemu_irq cpu_irq;
+    qemu_irq cpu_irq[2];

     if (!cpu_model) {
         cpu_model = "cortex-a9";
     }

-    cpu = cpu_arm_init(cpu_model);
-    if (!cpu) {
-        fprintf(stderr, "Unable to find CPU definition\n");
-        exit(1);
+    for (n = 0; n < smp_cpus; n++) {
+        cpus[n] = cpu_arm_init(cpu_model);
+        if (!cpus[n]) {
+            fprintf(stderr, "Unable to find CPU definition\n");
+            exit(1);
+        }
+        irqp = arm_pic_init_cpu(cpus[n]);
+        cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ];
+        /* FIXME: handle this somewhere central */
+        object_property_add_child(container_get(qdev_get_machine(),
+                                        "/unattached"),
+                                    g_strdup_printf("cpu[%d]", n),
+                                    OBJECT(cpus[n]), NULL);
     }
-    irqp = arm_pic_init_cpu(cpu);
-    cpu_irq = irqp[ARM_PIC_CPU_IRQ];

full tree available at git://developer.petalogix.com/public/qemu.git
for-upstream/zynq-boot.next. Theres a few more devels until I have a
sendable series, but the bits your worried about in this convo are
there now if you want more info.

Regards,
Peter

> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg