| Message ID | 1344925665-24626-1-git-send-email-peter.crosthwaite@petalogix.com |
|---|---|
| State | New |
| Headers | show |
"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
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 >
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
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
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) {
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(-)