[Xcb] bug #8208: proposed patches

[Jamey Sharp]

Thanks for the review, Ian.

On Fri, Sep 15, 2006 at 07:33:18AM -0700, Ian Osgood wrote:
> On Sep 15, 2006, at 2:29 AM, Jamey Sharp wrote:
> >I chatted further with Bart about bug #8208 the other day, and he
> >argued that the "minimal API" for it that I had proposed wasn't
> >actually minimal enough. Following his suggestions, I've implemented
> >the function XCBConnectionHasError, which returns only a boolean of
> >whether the connection is shut down due to some error.
> 
> If this is boolean, applies to the connection, and means your
> connection is no longer valid; then how about naming the function
> "XCBDisconnected()"?

Um... I guess that works too... Why?

As a general matter, I like having "is" or "has" in functions that
return boolean, which is why I named it this way.

> So the idea is that if XCBConnect() == 1, you can check errno for more
> details?

The idea is we'll add more API later that lets you find out what went
wrong, but we need to take the time to design that API. For now, you get
no indication of what actually went wrong, but you can at least find out
that you can't use that connection any more.

Note that there's absolutely no guarantee that errno was set by whatever
error occurred, nor that errno is set in the thread you're calling
XCBConnectionHasError from.

> Also, why return 1 when you could return a connection with has_error
> == 1?  Seems a bit more type-safe.

No, it's returning a valid pointer to some memory location containing 1,
not a pointer to address 1. It's just that if c->has_error is true,
there may not be any memory allocated for the rest of the XCBConnection
structure. XCBConnection is fairly big -- more than 8kB -- and I hate to
statically allocate an error object of that size.

Obviously the other refactoring would be to shrink XCBConnection, but I
think this is fine. You can trivially inspect all the public functions
that have access to the definition of XCBConnection and show that none
of them access memory after the has_error field if has_error is set...
unless has_error becomes set after it's checked, in which case the
memory certainly was valid.

> Shouldn't XCBConnectionHasError() also check for (c==NULL || (int) 
> c==1)?

As above, (int)c==1 is not a valid test -- c->has_error is. But the
question is whether this should be specified to consider a null pointer
equivalent to an error object.

For all other functions that take an XCBConnection, passing in a null
pointer is considered a programmer error. And because no XCB function
will ever return a null XCBConnection pointer with this patch, the
caller would have to have done something dumb to get a null pointer
here. So no, I think the app should segfault if it passes in null.

But I'm happy to listen to arguments to the contrary.

> In any case, the header comment for XCBConnect() should be  
> changed to indicate the function can now return NULL or 1 if there  
> are problems.

It now can't return NULL: it always returns an XCBConnection that you
can pass to any of XCB's other functions.

> The XCBPollForEvent() mutex used to protect get_event(), but now only  
> protects _xcb_in_read(). Is that OK?

Oops. No, it isn't. I was sleepy. I should have known there was a reason
I wrote the original function that weird way.

> Connection shutdown in XCBSendRequest() if an extension isn't present  
> is too harsh, isn't it? Same with exceeding max request length.

Both are preventable, so if your application can usefully respond to
those conditions, it should detect them before making the request and
avoid causing the error in the first place.

Real applications and toolkits should call XCBPrefetchExtensionData at
app startup for each extension they expect to use, preferably
immediately before making some other round-trip. Then they should check
that each of the extensions they asked for is present, using
XCBGetExtensionData, and arrange to not issue any requests to that
extension if it isn't present. That's why Bart and I believe it's OK to
shutdown if you *do* issue a request to an absent extension.

Exceeding the maximum request length is a slightly harder argument. It's
possible for the caller to check this, but they have to know how to
compute the total size of their request in order to compare it to the
return value of XCBGetMaximumRequestLength. While that's
straightforward, it's not hard to get wrong. I haven't been able to come
up with a generic way to help the programmer get it right, though.

I think the number of requests that even can get big enough for this to
matter is small enough that we'll see a library of special-purpose code
written for each one that splits the caller's arguments in a
request-specific way. RenderTrapezoids, for instance, can't have its
argument list split trivially: I believe this is what AddTrapezoids was
created for. Splitting PutImage requests, on the other hand, works very
differently, and it has been argued that PutImage should never use big
requests anyway.

--Jamey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.freedesktop.org/archives/xcb/attachments/20060915/7f4b8c44/attachment.pgp