Bug 41443 - Fatal error message when we close libX11 window application.
Summary: Fatal error message when we close libX11 window application.
Status: RESOLVED FIXED
Alias: None
Product: XCB
Classification: Unclassified
Component: Library (show other bugs)
Version: unspecified
Hardware: Other Solaris
: medium normal
Assignee: Arvind Umrao
QA Contact: xcb mailing list dummy
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-10-04 04:15 UTC by Arvind Umrao
Modified: 2012-01-11 09:02 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Patch fix (1.67 KB, patch)
2011-10-05 00:14 UTC, Arvind Umrao
Details | Splinter Review
Patch fix (1.41 KB, application/octet-stream)
2011-10-17 03:42 UTC, Arvind Umrao
Details
Patch fix (6.44 KB, patch)
2011-10-18 21:40 UTC, Arvind Umrao
Details | Splinter Review

Description Arvind Umrao 2011-10-04 04:15:29 UTC
When we close any libX11 window application, we get following fatal error message. Fatal error message comes only with libX11 which are based on xcb.

XIO:  fatal IO error 11 (Resource temporarily unavailable) on X server ":0.0"
      after 14 requests (13 known processed) with 0 events remaining.


Error is simply because I quit the application, without exiting while loop of 
XNextEvent. Fatal error will not occur if we process Window Close Event of Window-Manager through event handler so XNextEvent does not fail, with adding following line

Atom delWindow = XInternAtom( d, "WM_DELETE_WINDOW", 0 );
XSetWMProtocols(d , w, &delWindow, 1);

 while(1) {
     XNextEvent(d, &e);
...
     if(e.type==ClientMessage)
        {
        printf("Client Messages\n");
        break;
        }
....
   }



XCB should give same error message as our legacy xlib was giving. Fatal error message is very confusing to developers, when they run simple 20 lines of following test sample. 



Step to reproduce the problem
Compile following code with cc test.c -lX11 
then run ./a.out
close the application by clicking X icon


 #include <X11/Xlib.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>

 int main(void) {
   Display *d;
   Window w;
   XEvent e;
   char *msg = "Hello, World!";
   int s;

                        /* open connection with the server */
   d = XOpenDisplay(NULL);
   if (d == NULL) {
     fprintf(stderr, "Cannot open display\n");
     exit(1);
   }

   s = DefaultScreen(d);

                        /* create window */
   w = XCreateSimpleWindow(d, RootWindow(d, s), 10, 10, 200, 200, 1,
                           BlackPixel(d, s), WhitePixel(d, s));

                        /* select kind of events we are interested in */
   XSelectInput(d, w, ExposureMask | KeyPressMask);

                        /* map (show) the window */
   XMapWindow(d, w);

                        /* event loop */
   while (1) {
     XNextEvent(d, &e);
                        /* draw or redraw the window */
     if (e.type == Expose) {
       XFillRectangle(d, w, DefaultGC(d, s), 20, 20, 10, 10);
       XDrawString(d, w, DefaultGC(d, s), 50, 50, msg, strlen(msg));
     }
                        /* exit on key press */
     if (e.type == KeyPress)
       break;
   }

                        /* close connection to server */
   XCloseDisplay(d);

   return 0;
 }
Comment 1 Arvind Umrao 2011-10-05 00:14:30 UTC
Created attachment 51992 [details] [review]
Patch fix

XCB should give same error messages as legacy libX11 were. In order to fix this issue, I have set right error number and made code similar to lib11:XlibInt.c:_XRead: ESET(EPIPE). I believe xcb_in.c is the best place to fix this error,rather than libX11:xcb_io.c. KCB was returning errno EAGAIN, I made it return EPIPE to make it similar to legacy libX11.
Comment 2 Rami Ylimaki 2011-10-05 00:48:30 UTC
Review of attachment 51992 [details] [review]:

You shouldn't set the EPIPE error. The _xcb_conn_shutdown function already signals the connection errors. So instead of checking for EPIPE, you could introduce a version of _xcb_conn_shutdown that would make it possible to give a specific value for xcb_connection_t::has_error. This value could then be used to display different error messages in IO error handler.
Comment 3 Arvind Umrao 2011-10-05 02:45:11 UTC
(In reply to comment #2)
> Review of attachment 51992 [details] [review]:
> 
> You shouldn't set the EPIPE error. The _xcb_conn_shutdown function already
> signals the connection errors. So instead of checking for EPIPE, you could
> introduce a version of _xcb_conn_shutdown that would make it possible to give a
> specific value for xcb_connection_t::has_error. This value could then be used
> to display different error messages in IO error handler.

good review. Thanks 

But right now XCB gives message number EAGAIN. I do not think EAGAIN is very appropriate error message when connection is already broken. If you think it is correct error message, then I have another fix, just change default io error hander of linX11, so that it does not generate fatal error for error case EAGAIN.

Right now in XCB, xcb_connection_t::has_error is harcoded to one. As you said xcb_connection_t::has_error is not very useful for IO handler and I do agree with you. 

I will be happy if somehow libX11 error hander does not print fatal error message, but print some sensible message.  

So we have two solutions, let me know which is best one
1) PATCH I have given, make errno= EPIPE , because legacy libX11 is also doing the same. 

2) If you think XCB is giving correct error number, then just change default io error hander of linX11, so that it does not generate fatal error for error case EAGAIN.
Comment 4 Rami Ylimaki 2011-10-05 03:10:06 UTC
(In reply to comment #3)
> But right now XCB gives message number EAGAIN. I do not think EAGAIN is very
> appropriate error message when connection is already broken. If you think it is
> correct error message, then I have another fix, just change default io error
> hander of linX11, so that it does not generate fatal error for error case
> EAGAIN.

When connection is closed by X server, read/recv returns zero and doesn't set any error numbers. The EAGAIN that you see must therefore come from an earlier system call.

> So we have two solutions, let me know which is best one
> 1) PATCH I have given, make errno= EPIPE , because legacy libX11 is also doing
> the same. 
> 
> 2) If you think XCB is giving correct error number, then just change default io
> error hander of linX11, so that it does not generate fatal error for error case
> EAGAIN.

Neither of these solutions is acceptable. You have no way of knowing which system call has set errno when IO error handler is called. Just set a proper error code in xcb_connection_t and use that (reuse has_error or add a new field).
Comment 5 Arvind Umrao 2011-10-05 03:55:51 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > But right now XCB gives message number EAGAIN. I do not think EAGAIN is very
> > appropriate error message when connection is already broken. If you think it is
> > correct error message, then I have another fix, just change default io error
> > hander of linX11, so that it does not generate fatal error for error case
> > EAGAIN.
> 
> When connection is closed by X server, read/recv returns zero and doesn't set
> any error numbers. The EAGAIN that you see must therefore come from an earlier
> system call.
> 

Yes you are right. EAGAIN is comming from pervious system calls. Thats why erron values are wrong. Our legacy libX11 hardcoded it with EPIPE.

> > So we have two solutions, let me know which is best one
> > 1) PATCH I have given, make errno= EPIPE , because legacy libX11 is also doing
> > the same. 
> > 
> > 2) If you think XCB is giving correct error number, then just change default io
> > error hander of linX11, so that it does not generate fatal error for error case
> > EAGAIN.
> 
> Neither of these solutions is acceptable. You have no way of knowing which
> system call has set errno when IO error handler is called. Just set a proper
> error code in xcb_connection_t and use that (reuse has_error or add a new
> field).



I do agree with you but I do not want to make big changes. libX11 is wrapper over xcb. libX11 default io hander print error message after filtering error variable erron. I am afraid changing any thing at libX11:XlibInt.c:_XDefaultIOError() is risky. So I have to set correct value of erron some where.

If you do not want me to set value of erron at xcb,  can I set it at libX11:xcb_io.c.?  
 
Like this way  
if(xcb_connection_has_error(dpy->xcb->connection))
{
  erron=EPIPE; //or ESET(EPIPE)   Even our legacy libX11 is also hardcoing it in same way
  
_XIOError(dpy);
}

Let me know if I should send you another patch with above changes.
Comment 6 Arvind Umrao 2011-10-17 03:42:44 UTC
Created attachment 52408 [details]
Patch fix
Comment 7 Arvind Umrao 2011-10-18 21:40:00 UTC
Created attachment 52507 [details] [review]
Patch fix
Comment 8 Julien Danjou 2012-01-11 09:02:13 UTC
Patch merged.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.