Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly
On Thu, May 30, 2013 at 05:05:51PM +0200, Laszlo Ersek wrote:
> On 05/30/13 15:27, Michael S. Tsirkin wrote:
> > Use the type-safe FWCfgState structure instead
> > of the unsafe void *.
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
> > hw/misc/pvpanic.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> > index 31e1b1d..1483f27 100644
> > --- a/hw/misc/pvpanic.c
> > +++ b/hw/misc/pvpanic.c
> > @@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
> > {
> > PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> > static bool port_configured;
> > -void *fw_cfg;
> > +FWCfgState *fw_cfg;
> >
> > memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
> > isa_register_ioport(dev, &s->io, s->ioport);
> >
>
> Doesn't this break your build? Lower down in the function there's
>
> fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
>
> and object_resolve_path() returns a pointer-to-Object, not
> pointer-to-FWCfgState.
I see, I think I should reorder the patches.
> But for starters I'm quite confused about how the unpatched function
> works. What it does amounts to:
>
> fw_cfg_add_file(object_resolve_path(...), ...);
>
> But, again object_resolve_path() returns pointer-to-Object. I'm checking
> "struct Object" in "include/qom/object.h", and it suggests that derived
> structs should embed Object as first member. However FWCfgState is *not*
> such a derived member. What's going on here?
>
> http://thread.gmane.org/gmane.comp.emulators.qemu/201544/focus=201564
> http://thread.gmane.org/gmane.comp.emulators.qemu/204452/focus=204450
>
> Laszlo
Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly
On 05/30/13 17:05, Laszlo Ersek wrote:
> On 05/30/13 15:27, Michael S. Tsirkin wrote:
>> Use the type-safe FWCfgState structure instead
>> of the unsafe void *.
>>
>> Signed-off-by: Michael S. Tsirkin
>> ---
>> hw/misc/pvpanic.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
>> index 31e1b1d..1483f27 100644
>> --- a/hw/misc/pvpanic.c
>> +++ b/hw/misc/pvpanic.c
>> @@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
>> {
>> PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
>> static bool port_configured;
>> -void *fw_cfg;
>> +FWCfgState *fw_cfg;
>>
>> memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
>> isa_register_ioport(dev, &s->io, s->ioport);
>>
>
> Doesn't this break your build? Lower down in the function there's
>
> fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
>
> and object_resolve_path() returns a pointer-to-Object, not
> pointer-to-FWCfgState.
Paolo explained the guts, but don't we still need a downcast here? (No
idea how to do that nicely in the object model du jour -- maybe
OBJECT_CHECK() or similar?)
Laszlo
Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly
Il 30/05/2013 18:03, Laszlo Ersek ha scritto:
> On 05/30/13 17:05, Laszlo Ersek wrote:
>> On 05/30/13 15:27, Michael S. Tsirkin wrote:
>>> Use the type-safe FWCfgState structure instead
>>> of the unsafe void *.
>>>
>>> Signed-off-by: Michael S. Tsirkin
>>> ---
>>> hw/misc/pvpanic.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
>>> index 31e1b1d..1483f27 100644
>>> --- a/hw/misc/pvpanic.c
>>> +++ b/hw/misc/pvpanic.c
>>> @@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
>>> {
>>> PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
>>> static bool port_configured;
>>> -void *fw_cfg;
>>> +FWCfgState *fw_cfg;
>>>
>>> memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
>>> isa_register_ioport(dev, &s->io, s->ioport);
>>>
>>
>> Doesn't this break your build? Lower down in the function there's
>>
>> fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
>>
>> and object_resolve_path() returns a pointer-to-Object, not
>> pointer-to-FWCfgState.
>
> Paolo explained the guts, but don't we still need a downcast here? (No
> idea how to do that nicely in the object model du jour -- maybe
> OBJECT_CHECK() or similar?)
Patch 2 addresses that.
Paolo
Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly
On 05/30/13 17:41, Paolo Bonzini wrote: > Il 30/05/2013 17:05, Laszlo Ersek ha scritto: >> But, again object_resolve_path() returns pointer-to-Object. I'm checking >> "struct Object" in "include/qom/object.h", and it suggests that derived >> structs should embed Object as first member. However FWCfgState is *not* >> such a derived member. What's going on here? >> >> http://thread.gmane.org/gmane.comp.emulators.qemu/201544/focus=201564 >> http://thread.gmane.org/gmane.comp.emulators.qemu/204452/focus=204450 > > FWCfgState embeds Object via SysBusDevice and DeviceState. Clearly an evil trick by a devious mind. Thanks :) Laszlo
Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly
Il 30/05/2013 17:05, Laszlo Ersek ha scritto: > But, again object_resolve_path() returns pointer-to-Object. I'm checking > "struct Object" in "include/qom/object.h", and it suggests that derived > structs should embed Object as first member. However FWCfgState is *not* > such a derived member. What's going on here? > > http://thread.gmane.org/gmane.comp.emulators.qemu/201544/focus=201564 > http://thread.gmane.org/gmane.comp.emulators.qemu/204452/focus=204450 FWCfgState embeds Object via SysBusDevice and DeviceState. Paolo
Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly
On 05/30/13 15:27, Michael S. Tsirkin wrote:
> Use the type-safe FWCfgState structure instead
> of the unsafe void *.
>
> Signed-off-by: Michael S. Tsirkin
> ---
> hw/misc/pvpanic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 31e1b1d..1483f27 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
> {
> PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> static bool port_configured;
> -void *fw_cfg;
> +FWCfgState *fw_cfg;
>
> memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
> isa_register_ioport(dev, &s->io, s->ioport);
>
Doesn't this break your build? Lower down in the function there's
fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
and object_resolve_path() returns a pointer-to-Object, not
pointer-to-FWCfgState.
But for starters I'm quite confused about how the unpatched function
works. What it does amounts to:
fw_cfg_add_file(object_resolve_path(...), ...);
But, again object_resolve_path() returns pointer-to-Object. I'm checking
"struct Object" in "include/qom/object.h", and it suggests that derived
structs should embed Object as first member. However FWCfgState is *not*
such a derived member. What's going on here?
http://thread.gmane.org/gmane.comp.emulators.qemu/201544/focus=201564
http://thread.gmane.org/gmane.comp.emulators.qemu/204452/focus=204450
Laszlo
[Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly
Use the type-safe FWCfgState structure instead
of the unsafe void *.
Signed-off-by: Michael S. Tsirkin
---
hw/misc/pvpanic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 31e1b1d..1483f27 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
{
PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
static bool port_configured;
-void *fw_cfg;
+FWCfgState *fw_cfg;
memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
isa_register_ioport(dev, &s->io, s->ioport);
--
MST
