From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45807) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VnPRv-00009G-2B for qemu-devel@nongnu.org; Mon, 02 Dec 2013 04:05:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VnPRo-0006xi-1i for qemu-devel@nongnu.org; Mon, 02 Dec 2013 04:05:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:13568) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VnPRn-0006xM-Pr for qemu-devel@nongnu.org; Mon, 02 Dec 2013 04:05:43 -0500 Message-ID: <1385975126.2367.15.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Mon, 02 Dec 2013 11:05:26 +0200 In-Reply-To: <87eh5vzp44.fsf@blackfin.pond.sub.org> References: <1385718225-26379-1-git-send-email-armbru@redhat.com> <1385903630.23603.7.camel@localhost.localdomain> <529B526C.9020502@suse.de> <87eh5vzp44.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, chouteau@adacore.com, blauwirbel@gmail.com, kraxel@redhat.com, aliguori@amazon.com, Paolo Bonzini , edgar.iglesias@gmail.com, Andreas =?ISO-8859-1?Q?F=E4rber?= On Mon, 2013-12-02 at 08:30 +0100, Markus Armbruster wrote: > Andreas F=C3=A4rber writes: >=20 > > Am 01.12.2013 14:13, schrieb Marcel Apfelbaum: > >> On Fri, 2013-11-29 at 10:43 +0100, armbru@redhat.com wrote: > >>> From: Markus Armbruster > >>> > >>> 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 unle= ss > >>> the property may remain null. cannot_instantiate_with_device_add_y= et > >>> 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_use= r" > >>> series. > >>=20 > >> Even that I am not familiar with this code, I've checked all the cha= nges > >> and I agree with them. > >>=20 > >> Reviewed-by: Marcel Apfelbaum > >>=20 > >> Anyway, I do have a question: > >> Why not asserting on qdev_device_add if we have a pointer property? >=20 > This is a really good thought. In fact, it occurred to me, too. > However, see "unless the property may remain null" above: there are use= s > 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. >=20 > > 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 someth= ing > > stupid that we can easily prevent. ;) >=20 > 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. >=20 > > 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...? >=20 > /* FIXME: Remove opaque pointer properties. */ >=20 > /* Not a proper property, just for dirty hacks. TODO Remove it! */ >=20 > :) >=20 > >> 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. Althoug= h > > 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. >=20 > 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. It was merely a design (and understanding) question, if we prefer to enfo= rce such things and not rely on future work to comply with rules defined in c= omments. Though I am curios what others think about this specific scenario? Worst case scenario: the coder forgets about it, the reviewers don't catc= h this, the initialization code does not ensure the property is set and the device is added with an "unhealthy" state. But I suppose such a scena= rio would be caught early in the development cycle and is not a real issue.=20 Markus, thanks for the explanations, Marcel >=20 > [...]