Re: [PATCH 13/16] qdev: Make qdev_prop_set_drive() match the other helpers

From: Markus Armbruster <armbru@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: kwolf@redhat.com, berrange@redhat.com, ehabkost@redhat.com,
	qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com,
	pbonzini@redhat.com, jsnow@redhat.com
Subject: Re: [PATCH 13/16] qdev: Make qdev_prop_set_drive() match the other helpers
Date: Mon, 08 Jun 2020 07:20:40 +0200	[thread overview]
Message-ID: <87v9k2xhef.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <71a59dd5-9cea-9474-e318-4da3cb4f107b@redhat.com> ("Philippe =?utf-8?Q?Mathieu-Daud=C3=A9=22's?= message of "Fri, 5 Jun 2020 17:33:07 +0200")

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 6/5/20 4:56 PM, Markus Armbruster wrote:
>> qdev_prop_set_drive() can fail.  None of the other qdev_prop_set_FOO()
>> can; they abort on error.
>> 
>> To clean up this inconsistency, rename qdev_prop_set_drive() to
>> qdev_prop_set_drive_err(), and create a qdev_prop_set_drive() that
>> aborts on error.
>> 
>> Coccinelle script to update callers:
>> 
>>     @ depends on !(file in "hw/core/qdev-properties-system.c")@
>>     expression dev, name, value;
>>     symbol error_abort;
>>     @@
>>     -    qdev_prop_set_drive(dev, name, value, &error_abort);
>>     +    qdev_prop_set_drive(dev, name, value);
>
> Why not open-code qdev_prop_set_drive_err(..., &error_abort)?

Consistency with qdev_prop_set_chr() and qdev_prop_set_netdev().

My starting point was "what makes block backends so different that they
need error handling where nothing else does?"

After a considerable amount of digging, my answer is "nothing".
qdev_prop_set_drive(), qdev_prop_set_chr() and qdev_prop_set_netdev()
can all run into errors.  On closer examination, all programming errors
(thus &error_abort), except for "backend is already in use", and to
trigger that one, you have to get creative and steal the backend for
another purpose, e.g. with -global.  This is the abridged version of a
longwinded argument I didn't want to make in this series, so I left the
error handling alone.

In the longer run, I want qdev_prop_set_drive_err() to die.

>
>> 
>>     @@
>>     expression dev, name, value, errp;
>>     @@
>>     -    qdev_prop_set_drive(dev, name, value, errp);
>>     +    qdev_prop_set_drive_err(dev, name, value, errp);
>> 
> [...]