[PATCH weston-ivi-shell v4 3/9] ivi-shell supports a type of shell for In-Vehicle Infotainment system.

[Pekka Paalanen]

On Tue, 20 May 2014 14:05:10 +0900
Nobuhiko Tanibata <nobuhiko_tanibata at xddp.denso.co.jp> wrote:

> Hi pq,
> 
> I applied your comments except for several ones,
> 
> >> +    struct ivi_shell *shell = wl_resource_get_user_data(resource);
> >> +    struct ivi_shell_surface *ivisurf = NULL;
> >> +    struct weston_layout_surface *layout_surface = NULL;
> >> +    struct weston_surface *es = 
> >> wl_resource_get_user_data(surface_resource);
> >> +    struct wl_resource *res;
> >> +    int32_t warn_idx = -1;
> >> +
> >> +    if (es != NULL) {
> >> +        layout_surface = weston_layout_surfaceCreate(es, id_surface);
> >> +        if (layout_surface == NULL)
> >> +            warn_idx = 1;
> >> +    } else {
> >> +        warn_idx = 0;
> > 
> > I think this case would be a compositor internal error, not a client
> > error. Userdata of a surface resource should never be NULL. I.e.
> > assert() kind of thing would be more approriate.
> > 
> 
> I expect it would send warning to client. So I implement it like the 
> above.

Is there really a way that a client (malfunction) could cause this?

I don't think so. Weston core always creates a weston_surface for a
wl_surface, and if it ever does not or it disappears, you have a
Weston bug or memory corruption. So I really don't think warning
the client is a) sufficient, nor b) relevant as the client simply
cannot have anything to do with triggering this. There is also
nothing the client could do to avoid or remedy it.

I believe this fits well on the definition of an internal error.

I know you have much higher demands for dealing with internal
errors than Weston usually does, so I would suggest using your own
assert mechanism, which just gets rid of the client via the
wl_display::error event. I think it would simplify your code. Do
you think it would work for you?

It would also serve a documentary purpose just like the plain
assert() does: this condition is not expected to fail, and if it
fails, there is a bug in *this* program somewhere, and not in its
input (clients).

Also, Weston already uses lots of plain asserts all over. Are those
not a concern to you?

> >> +    init_ivi_shell(ec, shell);
> >> +    weston_layout_initWithCompositor(ec);
> >> +
> >> +    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;
> >> +    }
> > 
> > You are not setting compositor->shell_interface, but OTOH those are
> > only used for Xwayland, which probably doesn't make sense here.
> 
> I was advised by kritian before it shall not support other shell in 
> ivi-shell at the same time.

Sorry?

Sure, I understand you do not want/cannot to have any other shell
loaded in Weston when ivi-shell is, but this is not about that.

This is about compositor->shell_interface, which is a set of vfuncs
set by the shell currently loaded. However, these vfuncs are only
used by the XWayland support (XWM), and so might not be necessary.
It's not a different shell, it is just about XWayland using the
current shell.

That was just an observation. I don't think you need to change
anything here, but if you want, you could add a comment that
XWayland is not supported here. That is all.


Thanks,
pq