Re: [Qemu-devel] [PATCH v3 12/14] pvpanic: add API to access io port

[Andreas Färber]

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

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