| Message ID | 1374681580-17439-13-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/24/13 18:02, Michael S. Tsirkin wrote: > Add API to find pvpanic device and get its io port. > Will be used to fill in guest info structure. > +uint16_t pvpanic_port(void) > +{ > + Object *o = object_resolve_path_type("", TYPE_ISA_PVPANIC_DEVICE, NULL); > + PVPanicState *s; > + if (!o) { > + return 0; > + } return object_property_get_int(o, "ioport"); } Then you don't need PVPanicState access and can place the code into acpi-build.c. cheers, Gerd
On Thu, Jul 25, 2013 at 12:29:52PM +0200, Gerd Hoffmann wrote: > On 07/24/13 18:02, Michael S. Tsirkin wrote: > > Add API to find pvpanic device and get its io port. > > Will be used to fill in guest info structure. > > > +uint16_t pvpanic_port(void) > > +{ > > + Object *o = object_resolve_path_type("", TYPE_ISA_PVPANIC_DEVICE, NULL); > > + PVPanicState *s; > > + if (!o) { > > + return 0; > > + } > > return object_property_get_int(o, "ioport"); > } > > Then you don't need PVPanicState access and can place the code into > acpi-build.c. > > cheers, > Gerd I can change the implementation but I don't think it's a good idea to copy property names around: it's too fragile, compiler won't warn us if we change the name or value semantics, or make a mistake in acpi-build.c Same applies to TYPE_ISA_PVPANIC_DEVICE: better to expose an API than expose the type externally.
On Thu, Jul 25, 2013 at 01:55:26PM +0300, Michael S. Tsirkin wrote: > On Thu, Jul 25, 2013 at 12:29:52PM +0200, Gerd Hoffmann wrote: > > On 07/24/13 18:02, Michael S. Tsirkin wrote: > > > Add API to find pvpanic device and get its io port. > > > Will be used to fill in guest info structure. > > > > > +uint16_t pvpanic_port(void) > > > +{ > > > + Object *o = object_resolve_path_type("", TYPE_ISA_PVPANIC_DEVICE, NULL); > > > + PVPanicState *s; > > > + if (!o) { > > > + return 0; > > > + } > > > > return object_property_get_int(o, "ioport"); > > } > > > > Then you don't need PVPanicState access and can place the code into > > acpi-build.c. > > > > cheers, > > Gerd > > I can change the implementation but I don't think it's > a good idea to copy property names around: > it's too fragile, compiler won't warn us if we > change the name or value semantics, or make > a mistake in acpi-build.c And even within pvpanic.c, this will make me copy-paste the "ioport" string or create yet another macro. s = ISA_PVPANIC_DEVICE(o); return s->ioport; seems nicer. > Same applies to TYPE_ISA_PVPANIC_DEVICE: better > to expose an API than expose the type externally. > -- > MST
Hi, > I can change the implementation but I don't think it's > a good idea to copy property names around: > it's too fragile, compiler won't warn us if we > change the name or value semantics, I'm not worried. Changing the strings will break the command line interface too (qemu -device pvpanic,ioport=...), so that isn't going to happen. cheers, Gerd
On Thu, Jul 25, 2013 at 01:05:12PM +0200, Gerd Hoffmann wrote: > Hi, > > > I can change the implementation but I don't think it's > > a good idea to copy property names around: > > it's too fragile, compiler won't warn us if we > > change the name or value semantics, > > I'm not worried. Changing the strings will break the command line > interface too (qemu -device pvpanic,ioport=...), so that isn't going to > happen. > > cheers, > Gerd What will catch this breakage? There are 0 users actually tweaking the port number so I'm sure no one will notice this. In any case, catching errors at compile time is much better than at runtime. What exactly are advantages of duplicating property names in this way? I don't see any.
On 07/25/13 13:22, Michael S. Tsirkin wrote: > On Thu, Jul 25, 2013 at 01:05:12PM +0200, Gerd Hoffmann wrote: >> Hi, >> >>> I can change the implementation but I don't think it's >>> a good idea to copy property names around: >>> it's too fragile, compiler won't warn us if we >>> change the name or value semantics, >> >> I'm not worried. Changing the strings will break the command line >> interface too (qemu -device pvpanic,ioport=...), so that isn't going to >> happen. >> >> cheers, >> Gerd > > What will catch this breakage? > There are 0 users actually tweaking the port > number so I'm sure no one will notice this. > > In any case, catching errors at compile time > is much better than at runtime. > > What exactly are advantages of duplicating > property names in this way? I don't see any. You don't need access to pvpanic internals then and thus the code can be moved over to the acpi generator. At least in this case where all info needed is already available via properties. cheers, Gerd
On Thu, Jul 25, 2013 at 02:03:33PM +0200, Gerd Hoffmann wrote: > On 07/25/13 13:22, Michael S. Tsirkin wrote: > > On Thu, Jul 25, 2013 at 01:05:12PM +0200, Gerd Hoffmann wrote: > >> Hi, > >> > >>> I can change the implementation but I don't think it's > >>> a good idea to copy property names around: > >>> it's too fragile, compiler won't warn us if we > >>> change the name or value semantics, > >> > >> I'm not worried. Changing the strings will break the command line > >> interface too (qemu -device pvpanic,ioport=...), so that isn't going to > >> happen. > >> > >> cheers, > >> Gerd > > > > What will catch this breakage? > > There are 0 users actually tweaking the port > > number so I'm sure no one will notice this. > > > > In any case, catching errors at compile time > > is much better than at runtime. > > > > What exactly are advantages of duplicating > > property names in this way? I don't see any. > > You don't need access to pvpanic internals then and thus the code can be > moved over to the acpi generator. At least in this case where all info > needed is already available via properties. > > cheers, > Gerd We'll have to disagree here. There's no access to internals with an API. I prefer using APIs, since they are compiler-checked.
Am 25.07.2013 14:23, schrieb Michael S. Tsirkin: > On Thu, Jul 25, 2013 at 02:03:33PM +0200, Gerd Hoffmann wrote: >> On 07/25/13 13:22, Michael S. Tsirkin wrote: >>> On Thu, Jul 25, 2013 at 01:05:12PM +0200, Gerd Hoffmann wrote: >>>>> I can change the implementation but I don't think it's >>>>> a good idea to copy property names around: >>>>> it's too fragile, compiler won't warn us if we >>>>> change the name or value semantics, >>>> >>>> I'm not worried. Changing the strings will break the command line >>>> interface too (qemu -device pvpanic,ioport=...), so that isn't going to >>>> happen. >>> >>> What will catch this breakage? >>> There are 0 users actually tweaking the port >>> number so I'm sure no one will notice this. >>> >>> In any case, catching errors at compile time >>> is much better than at runtime. >>> >>> What exactly are advantages of duplicating >>> property names in this way? I don't see any. >> >> You don't need access to pvpanic internals then and thus the code can be >> moved over to the acpi generator. At least in this case where all info >> needed is already available via properties. > > We'll have to disagree here. > There's no access to internals with an API. > I prefer using APIs, since they are compiler-checked. Sorry, I don't understand, is there a typo? A qdev "ioport" property is hardly internal, and QOM has an API to access it. For QOM properties we have ABI stability rules in place, not only for the textual command line: No one is allowed to change the type of a property in an incompatible way. And dropping it would be a command-line-incompatible change in this case. Properties don't magically disappear at runtime, and since you're writing this code you can rely on the property being present and the person attempting to remove it noticing it while testing. You're writing redundant code here, and it's not your device IIRC. I vaguely remember seeing you or Xen guys adding some check that the pvpanic device can only be instantiated once, right? In that case I would consider it best to add an object_property_add_child(qdev_get_machine(), "pvpanic", foo, NULL); call into pvpanic device creation, then you can access it from ACPI code without needing to know the type via obj = object_path_resolve_component(qdev_get_machine(), "pvpanic"); if (obj != NULL) { val = object_property_get_int(obj, "ioport", &err); assert_no_error(&err); } and pvpanic code remains untouched. Andreas
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c index 7bb49a5..bb403e9 100644 --- a/hw/misc/pvpanic.c +++ b/hw/misc/pvpanic.c @@ -122,6 +122,17 @@ void pvpanic_init(ISABus *bus) pvpanic_fw_cfg(dev, fw_cfg); } +uint16_t pvpanic_port(void) +{ + Object *o = object_resolve_path_type("", TYPE_ISA_PVPANIC_DEVICE, NULL); + PVPanicState *s; + if (!o) { + return 0; + } + s = ISA_PVPANIC_DEVICE(o); + return s->ioport; +} + static Property pvpanic_isa_properties[] = { DEFINE_PROP_UINT16("ioport", PVPanicState, ioport, 0x505), DEFINE_PROP_END_OF_LIST(), diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 76af5cd..e718a59 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -228,6 +228,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory); /* pvpanic.c */ void pvpanic_init(ISABus *bus); +uint16_t pvpanic_port(void); /* e820 types */ #define E820_RAM 1
Add API to find pvpanic device and get its io port. Will be used to fill in guest info structure. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/misc/pvpanic.c | 11 +++++++++++ include/hw/i386/pc.h | 1 + 2 files changed, 12 insertions(+)