[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add |
Date: |
Mon, 02 Dec 2013 08:30:35 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
Andreas Färber <address@hidden> writes:
> Am 01.12.2013 14:13, schrieb Marcel Apfelbaum:
>> On Fri, 2013-11-29 at 10:43 +0100, address@hidden wrote:
>>> From: Markus Armbruster <address@hidden>
>>>
>>> Pointer properties can be set only by code, not by device_add. A
>>> device with a pointer property can't work with device_add only unless
>>> the property may remain null. cannot_instantiate_with_device_add_yet
>>> needs to be set then. PATCH 1/2 sets it when needed and else
>>> documents why not. PATCH 2/2 documents this for future users of
>>> pointer properties.
>>>
>>> This applies on top of my "[PATCH v4 00/10] Clean up and fix no_user"
>>> series.
>>
>> Even that I am not familiar with this code, I've checked all the changes
>> and I agree with them.
>>
>> Reviewed-by: Marcel Apfelbaum <address@hidden>
>>
>> Anyway, I do have a question:
>> Why not asserting on qdev_device_add if we have a pointer property?
This is a really good thought. In fact, it occurred to me, too.
However, see "unless the property may remain null" above: there are uses
of pointer properties that do *not* make the device unusable with
device_add. We even have an example: etraxfs,pic; see PATCH 1/1. It's
a sysbus device, so it's unavailable anyway. But there certainly could
be a device with an optional property that does not and should not have
cannot_instantiate_with_device_add_yet set.
> When we do device_add / device-add, the guest is usually running and we
> shouldn't kill a running guest just because the user is trying something
> stupid that we can easily prevent. ;)
You have a point on assert(bad_input), but this would be
assert(programming_error), where the error is "device doesn't have
cannot_instantiate_with_device_add_yet set". I'm advocating to be
ruthless with programming error asserts.
> The alternative BTW is dropping all those pointer properties and
> replacing them with link<> properties. Paolo tried that for the OMAP
> timers once but I fear that series was never picked up...?
/* FIXME: Remove opaque pointer properties. */
/* Not a proper property, just for dirty hacks. TODO Remove it! */
:)
>> Instead of checking only cannot_instantiate_with_device_add_yet,
>> we can go over properties and if we have a pointer property, assert or
>> return...
>
> Raising an error for certain property types may be an option. Although
> theoretically the existence of an incompatible property would not
> necessarily indicate incompatibility to instantiate the device, in
> practice I believe we don't have such excess properties.
We don't have them now. I hope we won't permit any new pointer
properties. If you guys want pointer property imply its owner's
cannot_instantiate_with_device_add_yet, even though it's not generally
necessary, I'm fine with that.
[...]