Re: [Qemu-devel] [PATCH v2] qom: Reject attempts to add a property that already exists

[Anthony Liguori <aliguori@us.ibm.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 8 October 2012 14:29, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> This is wrong.
>>
>> Container properties are added by the user.  You will turn a gracefully
>> failure (during hotplug) into an abort().
>
> No, it's turning a bug into an abort -- we don't handle trying to
> create two identically named properties correctly today.

Killing a guest because of something a user mistypes is not very friendly.

>
>> Please limit this to static properties as they are not added by a user.
>
> Adding two dynamic properties of the same name is also not
> going to work and we need to do something with it...

Raise an error.

> What is the code path for properties being added by a user?

qdev_device_add().

> If it's qdev_device_add() then that code presumably doesn't
> care about graceful failures because it passes NULL as an
> error pointer.

Then we should handle the error there gracefully.

> container_get() seems to assume that adding the
> child property will always succeed and will not do the right
> thing if there already exists a child property of the relevant
> name but wrong type.
>
> Basically it seems to me that any code which might actually
> be hit by this assert() rather needs examination and rewriting
> to handle the error case anyway...

There are only two cases that actually matter today:

1) static properties

2) qdev_device_add().

Yes, (2) is not doign error checking today.  It should.  I would be very
happy with an abort() in (1) since that's clearly a programming bug.

Regards,

Anthony Liguori

>
> -- PMM