On Wed, 30 Apr 2014 09:40:19 +0200 Marek Chalupa <mchqwe...@gmail.com> wrote:
> On 24 April 2014 14:45, Pekka Paalanen <ppaala...@gmail.com> wrote: > > > On Wed, 23 Apr 2014 14:39:48 +0200 > > Marek Chalupa <mchqwe...@gmail.com> wrote: > > > > > +/** > > > + * This function is called for error events > > > + * and idicates that in some object occured an error. > > > + * Difference between this function and display_fatal_error() > > > + * is that this one handles errors that will come in wire, whereas > > > + * display_fatal_error() is called for local errors. > > > + * > > > + * \param display > > > + * \param code error code > > > + * \param id id of the object that generated the error > > > + * \param intf protocol interface > > > + */ > > > static void > > > -wl_display_fatal_error(struct wl_display *display, int error) > > > +display_protocol_error(struct wl_display *display, int code, > > > + unsigned int id, const struct wl_interface *intf) > > > { > > > + struct wl_event_queue *iter; > > > + int err; > > > + > > > + /* set correct errno */ > > > + if (wl_interface_equal(intf, &wl_display_interface)) { > > > + switch (code) { > > > + case WL_DISPLAY_ERROR_INVALID_OBJECT: > > > + case WL_DISPLAY_ERROR_INVALID_METHOD: > > > + err = EINVAL; > > > + break; > > > + case WL_DISPLAY_ERROR_NO_MEMORY: > > > + err = ENOMEM; > > > + break; > > > + default: > > > + err = EFAULT; > > > + } > > > + } else { > > > + err = EPROTO; > > > + } > > > + > > > + if (display->last_error) > > > + return; > > > > This 'if' could be the very first thing. Doesn't matter. > > > > > + > > > pthread_mutex_lock(&display->mutex); > > > - display_fatal_error(display, error); > > > + > > > + display->last_error = err; > > > + > > > + display->protocol_error.code = code; > > > + display->protocol_error.id = id; > > > + display->protocol_error.interface = intf; > > > + > > > + wl_list_for_each(iter, &display->event_queue_list, link) > > > + pthread_cond_broadcast(&iter->cond); > > > > What is the purpose of this pthread_cond_broadcast()? I am not too > > familiar with the logic here, but looks like this is something new you > > added, and I'm not sure it belongs with the addition of > > wl_display_get_protocol_error(). > > > > If this is fixing something, I feel like it should be a separate patch, > > since I can't see how adding wl_display_get_protocol_error() would add > > the need for this. Either the need was there to begin with and this > > bug fix should get into the stable releases, or it's not needed. Am I > > missing something? > > > > > This broadcast is not new, so I didn't touch it. So far as I see into it it I was confused about it because your patch never removes any lines doing pthread_cond_broadcast(), but it does add it. Ok, now I see it. Originally, display_fatal_error() does it, and wl_display_fatal_error() called it. Your patch renames wl_display_fatal_error to display_protocol_error and removes the call to display_fatal_error(), so you copy back the relevant parts of display_fatal_error(). > should > wake up all the sleeping event queues (when there's more of them than the > default one - in multi-threaded environment) so that they can receive the > error > and notify the client. > > relevant commits: > 33b7637b4500a682018b503837b8aca9afae36f2 > 385fe30e8b144a968aa88c6546c2ef247771b3d7 > > However, I don't see any pthread_cond_{wait, timedwait} for this condition > anywhere in the code and that's weird... Marking the 'cond' member deprecated, the compiler tells me it is used in the following places in current master: CC src/libwayland_client_la-wayland-client.lo src/wayland-client.c: In function 'display_fatal_error': src/wayland-client.c:113:3: warning: 'cond' is deprecated (declared at src/wayland-client.c:75) [-Wdeprecated-declarations] ** pthread_cond_broadcast(&iter->cond); src/wayland-client.c: In function 'wl_event_queue_init': src/wayland-client.c:128:2: warning: 'cond' is deprecated (declared at src/wayland-client.c:75) [-Wdeprecated-declarations] ** pthread_cond_init(&queue->cond, NULL); src/wayland-client.c: In function 'wl_event_queue_release': src/wayland-client.c:143:2: warning: 'cond' is deprecated (declared at src/wayland-client.c:75) [-Wdeprecated-declarations] ** pthread_cond_destroy(&queue->cond); src/wayland-client.c: In function 'queue_event': src/wayland-client.c:979:3: warning: 'cond' is deprecated (declared at src/wayland-client.c:75) [-Wdeprecated-declarations] ** pthread_cond_signal(&queue->cond); You're right, and I cannot see a related mutex, either. Maybe it is unneeded or we have a hard to trigger bug here. Nice find. Thanks, pq _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel