[Xcb] [PATCH 2/7] Ensure that there's still a valid connection to the X server before attempting to send a request.

[Uli Schlachter]

On 29.02.2012 21:28, Alex Plotnick wrote:
> At or around Wed, 29 Feb 2012 20:42:04 +0100, Uli Schlachter said:
>> On 29.02.2012 17:59, Alex Plotnick wrote:
> 
>>> diff --git a/src/ext.c b/src/ext.c
>>> index 2d622d9..6475850 100644
>>> --- a/src/ext.c
>>> +++ b/src/ext.c
>>> @@ -113,6 +113,10 @@ xpybExt_send_request(xpybExt *self, PyObject *args, PyObject *kw)
>>>  	    return NULL;
>>>  	}
>>>  
>>> +    /* Check the connection */
>>> +    if (xpybConn_invalid(self->conn))
>>> +	return NULL;
>>> +
>>>      /* Set up request structure */
>>>      xcb_req.count = 2;
>>>      xcb_req.ext = (self->key != (xpybExtkey *)Py_None) ? &self->key->key : 0;
>>
>> I don't know much about xpyb's internals, but what exactly is the problem that
>> this patch fixes? Looking at the code, I'd expect the code to not do anything
>> bad with an error connection.
> 
> A few lines down, there's a call to xcb_send_request(self->conn->conn, ...).
> When that first argument is NULL, it dumps core. I consider that bad.
> 
> This isn't a hypothetical problem; I fixed this because it was actually
> crashing there. There's similar guard code in some of the other functions,
> but it was missing here.

Ok, I can understand that NULL is bad here. I just don't know where that NULL
would be coming from, because when an X11 connection breaks, that doesn't
magically turn the xcb_connection_t* into a NULL pointer.

I'll just assume that xpyb does some magic which causes this.

>> IMHO some of the other patches could need a more verbose commit message, too.
> 
> I didn't really feel like the trivial ones needed much detail, but if
> others disagree, I'd be happy to provide it. Which ones in particular
> did you have in mind?

Saying that this one fixes an xcb_send_request(NULL, ....) makes it already
clearer what to look for. A hint on which function set self->conn->conn = NULL
would make me think "That sounds good, he must know what he is doing".

Patch 3 could say where that NULL is coming from, too. I assume that a garbage
collection callback is involved which sets this to NULL, but it'd be nice to
have this assumption confirmed.

I can't say much about patch 7. Code in a language that I don't know that
generates code in a language that I don't know. Meh. :-)

I could provide some "Reviewed-By"s, but since I don't know much about xpyb and
haven't looked into the actual code, I better don't.

>> Now let's figure out who the resident xpyb expert is and wake him up. :-)
> 
> I'm certainly not an xpyb (or xcb) expert; I'm just an interested user
> that ran into some problems, and did the best I could to fix them. I'd
> appreciate feedback from someone that knows the internals better than
> I do.

There are two kinds of experts. The original authors who invented it and the
interested users who got sucked in. You know that you got sucked in when someone
says you are an expert, but fixing problems in the code generator is also a hint
in that direction. :-)

Uli

-- 
- Buck, when, exactly, did you lose your mind?
- Three months ago. I woke up one morning married to a pineapple.
  An ugly pineapple... But I loved her.