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

[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.

> >> +
> >> +    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