[waffle] [PATCH 07/12] waffle: support platform-specific information

[Emil Velikov]

On 21 April 2016 at 21:26, Frank Henigman <fjhenigman at google.com> wrote:
> On Fri, Jan 8, 2016 at 7:44 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 6 January 2016 at 21:56, Frank Henigman <fjhenigman at google.com> wrote:
>>> Add a platform hook so platform-specific information can be included
>>> by waffle_display_info_json().
>>>
>>> Signed-off-by: Frank Henigman <fjhenigman at google.com>
>>> ---
>>>  src/waffle/api/waffle_display.c  | 10 +++++++++-
>>>  src/waffle/core/wcore_platform.h |  4 ++++
>>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/waffle/api/waffle_display.c b/src/waffle/api/waffle_display.c
>>> index 7abe2ef..d6988ac 100644
>>> --- a/src/waffle/api/waffle_display.c
>>> +++ b/src/waffle/api/waffle_display.c
>>> @@ -367,8 +367,16 @@ waffle_display_info_json(struct waffle_display *self, bool platform_too)
>>>
>>>      json_appendv(jj, "{", "generic", "{", "");
>>>      add_generic_info(jj, wc_self->current_context);
>>> -    json_appendv(jj, "}", "}", "");
>>> +    json_append(jj, "}");
>>>
>>> +    if (platform_too) {
>>> +        json_appendv(jj, "platform", "{", "");
>>> +        if (api_platform->vtbl->display.info_json)
>>> +            api_platform->vtbl->display.info_json(wc_self, jj);
>> The rest of waffle tends to set UNSUPPORTED_ON_PLATFORM if the backend
>> is missing the vfunc.
>
> I'm reluctant to set an error for something that isn't clearly an
> error (it might just be that a backend doesn't have platform-specific
> details) because it could add a burden to the user to distinguish this
> case from definite errors.
With all respect I have to disagree. Error checking/handling is not a
'burden'. Even if some choose to ignore it that doesn't make it less
relevant ;-)

Obviously things would be way easier/cleaner if things were split -
generic info vs platform specific (or even finer). As-is, with all of
them in one piece, no error-checking or UNSUPPORTED_ON_PLATFORM one
gets badly formatted/corrupted json. Thus the user has no way of
knowing if things failed for reason some, or the setup simply lacks
the information Y that they need.
</minirant>

>  If someone does need to act on the
> presence or absence or platform-specifics, they can always examine the
> json.
Personally I'd just discard the check and use the function
unconditionally. For platforms that I'm unsure I'll just put a uniform
stub alike "not-implemented", thus any users parsing the data will
have clear feedback.

Does that sound reasonable ?
Emil