[Xcb] [PATCH] Added more error states and removed global error_connection

[Arvind Umrao]

On 11/ 8/11 08:18 PM, Uli Schlachter wrote:
> Wow, thanks.
>
> Patch looks good to me. I don't think my opinion matters much here, but since
> you asked for it:
>
> Reviewed-by: Uli Schlachter<psychon at znc.in>
>
> (Plus some small comment below, but I don't care much about that one)
>
>> TBD:
>> I will segregate errors states in a separate header file and try to
>> provide more precise error states, in future. Also I will give patch
>> for libX11, in that patch xcb_connection_t::has_error will be passed
>> to default io handler of libX11, This value can then be used for
>> displaying error messages.
>>
>> Signed-off-by: Arvind Umrao<arvind.umrao at oracle.com>
>> ---
>>   src/xcb.h      |   24 +++++++++++++++++++++---
>>   src/xcb_conn.c |   45 ++++++++++++++++++++++++++++++++++++---------
>>   src/xcb_in.c   |   14 +++++++-------
>>   src/xcb_out.c  |    4 ++--
>>   src/xcb_util.c |    4 ++--
>>   src/xcbint.h   |    7 ++++---
>>   6 files changed, 72 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/xcb.h b/src/xcb.h
>> index 3ee7965..df74762 100644
>> --- a/src/xcb.h
>> +++ b/src/xcb.h
>> @@ -69,6 +69,21 @@ extern "C" {
>>   /** X_TCP_PORT + display number = server port for TCP transport */
>>   #define X_TCP_PORT 6000
>>
>> +/** xcb connection errors for eg socket errors, pipe errors and other stream errors. */
>> +#define XCB_CONN_ERROR 1
>> +
>> +/** xcb connection shutdown because of extension not sppported */
>> +#define XCB_CONN_CLOSED_EXT_NOTSUPPORTED 2
>> +
>> +/** malloc(), calloc() and realloc() error upon failure, for eg ENOMEM */
>> +#define XCB_CONN_CLOSED_MEM_INSUFFICIENT 3
>> +
>> +/** Connection closed, exceeding request length that server accepts. */
>> +#define XCB_CONN_CLOSED_REQ_LEN_EXCEED 4
>> +
>> +/** Connection closed, error during parsing display string. */
>> +#define XCB_CONN_CLOSED_PARSE_ERR 5
>> +
>>   #define XCB_TYPE_PAD(T,I) (-(I)&  (sizeof(T)>  4 ? 3 : sizeof(T) - 1))
> Since this becomes part of the API, could someone who has a say double check if
> these names are fine?
>
> I'm just mentioning this since all these defines use a XCB_CONN_CLOSED_* prefix,
> except XCB_CONN_ERROR. So perhaps XCB_CONN_CLOSED_SOCKER_ERROR?
Initially it was Rami's idea to have two kinds of errors a) connection 
could not be established b) CON_CLOSED, connection could be established, 
but we need to shutdown because of insufficient memory, extension not 
supported, request length exceeding, etc.

For case a) there could be various reason of connection failure
a) Server not available
b) FD overflow
c) Client application terminated abruptly
d) socket errors
e) other stream and pipe errors

Most probably a user does not bother about detailed errors of connection 
establishment . However if you think we should be more precise then we 
have three options.

a) Add more error states like XCB_CONN_ERROR_SOCK, XCB_CONN_ERROR_FD, 
XCB_CONN_EPIPE,  XCB_CONN_ERROR_STREAM
b) Or add one more field at XCB connection structure, that will store 
precise connection issue.
c) Or use errno for finding precise connection issue. Keep in mind  for 
WIN32 environment, we have to use GetLastSocketError or GetLastError 
api. Rami is against this idea. He advocates for Xcb's own custom errors 
and I agree with him.

My plans for next phase of work to be more precise in errors, by adding 
more error states but in a separate header file. Defining many error 
states in a public xcb header file will be ugly.   There is a trade off 
between keeping minimum meaningful error states at xcb header file and 
adding more precise error states in a separate error header file. Also I 
have to change source code at some places to be more precise and 
accurate in error states.

All your suggestion are of great help, keep mentoring.

Thanks and Regards
-Arvind

>> @@ -396,15 +411,18 @@ int xcb_get_file_descriptor(xcb_connection_t *c);
>>   /**
>>    * @brief Test whether the connection has shut down due to a fatal error.
>>    * @param c: The connection.
>> - * @return 1 if the connection is in an error state; 0 otherwise.
>> + * @return>  0 if the connection is in an error state; 0 otherwise.
>>    *
>>    * Some errors that occur in the context of an xcb_connection_t
>>    * are unrecoverable. When such an error occurs, the
>>    * connection is shut down and further operations on the
>>    * xcb_connection_t have no effect.
>>    *
>> - * @todo Other functions should document the conditions in
>> - * which they shut down the connection.
>> + * @return XCB_CONN_ERROR, because of socket errors, pipe errors or other stream errors.
>> + * @return XCB_CONN_CLOSED_EXT_NOTSUPPORTED, when extension not supported.
>> + * @return XCB_CONN_CLOSED_MEM_INSUFFICIENT, when memory not available.
>> + * @return XCB_CONN_CLOSED_REQ_LEN_EXCEED, exceeding request length that server accepts.
>> + * @return XCB_CONN_CLOSED_PARSE_ERR, error during parsing display string.
>>    */
>>   int xcb_connection_has_error(xcb_connection_t *c);
>>
>> diff --git a/src/xcb_conn.c b/src/xcb_conn.c
>> index 3ab5385..aa0fbde 100644
>> --- a/src/xcb_conn.c
>> +++ b/src/xcb_conn.c
>> @@ -59,7 +59,9 @@ typedef struct {
>>       uint16_t length;
>>   } xcb_setup_generic_t;
>>
>> -const int error_connection = 1;
>> +static const int xcb_con_error = XCB_CONN_ERROR;
>> +static const int xcb_con_closed_mem_er = XCB_CONN_CLOSED_MEM_INSUFFICIENT;
>> +static const int xcb_con_closed_parse_er = XCB_CONN_CLOSED_PARSE_ERR;
>>
>