[PATCH weston-ivi-shell 04/15] ivi-shell supports a type of shell for In-Vehicle Infotainment system.

[Nobuhiko Tanibata]

2014-03-11 17:14 に Pekka Paalanen さんは書きました:
> On Tue, 11 Mar 2014 14:51:21 +0900
> Nobuhiko Tanibata <nobuhiko_tanibata at xddp.denso.co.jp> wrote:
> 
>> 2014-03-10 19:47 に Pekka Paalanen さんは書きました:
>> > On Thu,  6 Mar 2014 18:56:58 +0900
>> > Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp> wrote:
>> >
>> >> In-Vehicle Infotainment system traditionally manages surfaces with
>> >> global
>> >> identification. A protocol, ivi_application, supports such a feature
>> >> by
>> >> implementing a request, ivi_application::surface_creation defined in
>> >> ivi_application.xml. Additionally, it initialize a library,
>> >> weston-layout,
>> >> to manage properties of surfaces and group surfaces in layer. In
>> >> detail,
>> >> refer library, weston_layout.
>> >>
>> >> Signed-off-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp>
>> >> ---
>> >>  ivi-shell/ivi-shell.c | 333
>> >> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 333 insertions(+)
>> >>  create mode 100644 ivi-shell/ivi-shell.c
> 
> ...
> 
>> >> +static void
>> >> +init_ivi_shell(struct weston_compositor *ec, struct ivi_shell *shell)
>> >> +{
>> >> +    shell->compositor = ec;
>> >> +
>> >> +    wl_list_init(&ec->layer_list);
>> >> +    wl_list_init(&shell->list_surface);
>> >> +}
>> >> +
>> >> +/**
>> >> + * Initialization of ivi-shell. A library, weston_layout, is also
>> >> initialized
>> >> + * here by calling weston_layout_initWithCompositor.
>> >> + *
>> >> + */
>> >> +
>> >> +WL_EXPORT int
>> >> +module_init(struct weston_compositor *ec,
>> >> +            int *argc, char *argv[])
>> >> +{
>> >> +    struct ivi_shell  *shell = NULL;
>> >> +    struct weston_seat *seat = NULL;
>> >> +
>> >> +    shell = calloc(1, sizeof *shell);
>> >> +    if (shell == NULL) {
>> >> +        return -1;
>> >> +    }
>> >> +
>> >> +    init_ivi_shell(ec, shell);
>> >> +    weston_layout_initWithCompositor(ec);
>> >
>> > It's kind of surprising... this module's init requires another module
>> > to
>> > have been loaded before, otherwise this module will not load, right?
>> > I wonder if you get a sensible error message when that happens.
>> 
>> westen_layout is not a module to be loaded by ivi-shell. It is shared
>> library.
>> If it is not weston manner, I can link it statically. Please give me
>> your suggestion?
> 
> Oh, ok, that should be alright then. I just jumped into a conclusion
> that all .so's were plugins. I stand corrected. I read these patches
> one by one, when I can.
> 
> Is there a reason weston_layout should be a dynamic library? Do you
> expect it's ABI to be stable, and have a use case for swapping one of
> the two while keeping the other compiled binary intact?
> 
> If you are not committing to a stable library ABI, then I'd suggest not
> having it as a dynamic library to avoid any confusion and abuse; to
> avoid the risk of suddenly realizing you cannot change the API, because
> someone else is using it or having a different implementation of it.
> 

Hi pq,

I forget one important reason why weston_layout should be a dynamic 
library.
At first email, I enclosed a pdf. It said ivi-controller.so which is 
maintained in another repository of GENIVI. It will link weston_layout.
If I embed it in ivi-shell.so, I might need to set ivi-shell.so as 
shared library
to be linked to ivi-controller.so.

These APIs are based on GENIVI layer manager APIs. So it is stable and 
not be changed wihtout critical reason.

BR,
Nobuhiko

>> >> +
>> >> +    shell->destroy_listener.notify = shell_destroy;
>> >> +    wl_signal_add(&ec->destroy_signal, &shell->destroy_listener);
>> >> +
>> >> +    if (wl_global_create(ec->wl_display, &ivi_application_interface,
>> >> 1,
>> >> +                         shell, bind_ivi_application) == NULL) {
>> >> +        return -1;
>> >> +    }
>> >> +
>> >> +    wl_list_for_each(seat, &ec->seat_list, link) {
>> >> +        if (seat) {
>> >> +            shell->seat = seat;
>> >
>> > 'seat' cannot be NULL here. It seems you are assuming that there can be
>> > only one seat? Why?
>> >
>> 
>> Ideally, I agree. However I am in automotive software, and I have 
>> habit
>> to avoid NULL access as much as possible in case of the worst 
>> scenario.
> 
> Then put an assert(), though in this case even that would be too much.
> You can never get a NULL from wl_list_for_each(). You might get a bad
> pointer if the list is corrupted, but the odds of getting exactly NULL
> are diminishing even with corruption.
> 
> Silently ignoring NULLs where they should never appear may hide real
> problems in testing, and even in production you'd probably want to log
> it somehow to help post mortem.
> 
> 
> Thanks,
> pq