Daniel P. Berrangé <berra...@redhat.com> writes:
> On Thu, Sep 17, 2020 at 02:04:41PM +0200, Markus Armbruster wrote:
>> Greg Kurz <gr...@kaod.org> writes:
>>
>> > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l
>> > 60
>> >
>> > Manual inspecting the output of
>> >
>> > $ git grep -W object_property_get_uint -- :^{include,qom/object.c}
>> > ...
>> >
>> > seems to be showing that most users simply ignore errors (ie. pass NULL
>> > as 3rd argument). Then some users pass &error_abort and only a few
>> > pass a &err or errp.
>> >
>> > Assuming that users know what they're doing, I guess my proposal
>> > wouldn't bring much to the code base in the end... I'm not even
>> > sure now that it's worth changing the contract.
>>
>> We'd change
>>
>> val = object_property_get_uint(obj, name, &error_abort);
>>
>> to
>>
>> object_property_get_uint(obj, name, &val, &error_abort);
>>
>> which is not an improvement.
>>
>> Most of the ones passing NULL should probably pass &error_abort
>> instead. Doesn't change the argument.
>
> I wonder if we actually need to have an Error parameter at all for
> the getters. IIUC the only valid runtime error is probably the case
> where the property has not been set, and even then, I think properties
> usually have a default value that would be returned. All the other
> errors look like programmer errors.
>From the notes I took when I last hacked a trail through this jungle...
Failure modes of
object_property_get()
@name not found
@name permission, i.e. no ->get
->get() fails (how?)
object_property_get_{str,bool,int,uint}()
object_property_get_qobject()
object_property_get() with qobject output visitor
which see
prop value qobject not a string / bool / int / uint
object_property_get_enum()
@name not found
@typename doesn't match
object_property_get() with string output visitor
which see
qapi_enum_parse()
prop value not a value of enum @typename
object_property_get_link()
object_property_get_str()
which see
prop value does not resolve
I think most of these failures are obviously programming errors most of
the time.
Many callers treat failures as programming errors by passing
&error_abort.
Many callers ignore errors by passing NULL. I believe most of them
should really pass &error_abort instead. Fixing them is tedious,
because you have to check what would happen on error. If the answer is
"chaos", fix to pass &error_abort. But the answer can also be "works as
intended", or "beats me".
> IOW, can we remove the Error parameter and have all the o_p_get*
> method impls use error_abort.
If we fix the callers that should pass &error_abort to do so, we'll see
what remains, and whether we can drop the parameter.
> In the rare case where a caller needs to handle a property not
> being set, they can use object_property_find() as a test before
> invoking the getter.
I dislike "if (X() is going to fail) do something else; else X()".
I guess it could be okay for the narrow case of "property does not
exist".
> Of course requires someone motivated to audit all the case that
> are not using NULL or error_abort and decide whether the attempt
> at passing a local errp was actually justified or not.
We got one! Thanks, Greg :)
