[libxcb] Add comments about how _xcb_conn_ret_error() works

Submitted by Ran Benita on Jan. 18, 2014, 3:10 p.m.

Details

Message ID 1390057853-18850-1-git-send-email-ran234@gmail.com
State Accepted
Headers show

Not browsing as part of any series.

Commit Message

Ran Benita Jan. 18, 2014, 3:10 p.m.
If xcb_connect() fails, it doesn't return NULL. Instead, it always
returns an xcb_connection_t*, and the user should check for errors with
the xcb_connection_has_error() function. What this function does is
check if conn->has_error contains a non-zero error code, and returns it.

If an error did occur, xcb doesn't actually return a full
xcb_connection_t though, it just returns (xcb_connection_t *)
error_code. Since the 'has_error' field is the first, it is still
possible to check conn->has_error.

That last trick was not immediately obvious to me, so add some guiding
comments. This also ensures no one obliviously rearranges the struct.

Signed-off-by: Ran Benita <ran234@gmail.com>
---
 src/xcb_conn.c | 3 +++
 src/xcbint.h   | 1 +
 2 files changed, 4 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/xcb_conn.c b/src/xcb_conn.c
index 46390e1..00c458f 100644
--- a/src/xcb_conn.c
+++ b/src/xcb_conn.c
@@ -374,6 +374,9 @@  void _xcb_conn_shutdown(xcb_connection_t *c, int err)
 /* Return connection error state.
  * To make thread-safe, I need a seperate static
  * variable for every possible error.
+ * has_error is the first field in xcb_connection_t, so just
+ * return a casted int here; checking has_error (and only
+ * has_error) will be safe.
  */
 xcb_connection_t *_xcb_conn_ret_error(int err)
 {
diff --git a/src/xcbint.h b/src/xcbint.h
index b25f03b..67cf571 100644
--- a/src/xcbint.h
+++ b/src/xcbint.h
@@ -192,6 +192,7 @@  void _xcb_ext_destroy(xcb_connection_t *c);
 /* xcb_conn.c */
 
 struct xcb_connection_t {
+    /* This must be the first field; see _xcb_conn_ret_error(). */
     int has_error;
 
     /* constant data */

Comments

On 18.01.2014 16:10, Ran Benita wrote:
> If xcb_connect() fails, it doesn't return NULL. Instead, it always
> returns an xcb_connection_t*, and the user should check for errors with
> the xcb_connection_has_error() function. What this function does is
> check if conn->has_error contains a non-zero error code, and returns it.
> 
> If an error did occur, xcb doesn't actually return a full
> xcb_connection_t though, it just returns (xcb_connection_t *)
> error_code. Since the 'has_error' field is the first, it is still
> possible to check conn->has_error.
> 
> That last trick was not immediately obvious to me, so add some guiding
> comments. This also ensures no one obliviously rearranges the struct.
> 
> Signed-off-by: Ran Benita <ran234@gmail.com>

Reviewed-by: Uli Schlachter <psychon@znc.in>

> ---
>  src/xcb_conn.c | 3 +++
>  src/xcbint.h   | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/src/xcb_conn.c b/src/xcb_conn.c
> index 46390e1..00c458f 100644
> --- a/src/xcb_conn.c
> +++ b/src/xcb_conn.c
> @@ -374,6 +374,9 @@ void _xcb_conn_shutdown(xcb_connection_t *c, int err)
>  /* Return connection error state.
>   * To make thread-safe, I need a seperate static
>   * variable for every possible error.
> + * has_error is the first field in xcb_connection_t, so just
> + * return a casted int here; checking has_error (and only
> + * has_error) will be safe.
>   */
>  xcb_connection_t *_xcb_conn_ret_error(int err)
>  {
> diff --git a/src/xcbint.h b/src/xcbint.h
> index b25f03b..67cf571 100644
> --- a/src/xcbint.h
> +++ b/src/xcbint.h
> @@ -192,6 +192,7 @@ void _xcb_ext_destroy(xcb_connection_t *c);
>  /* xcb_conn.c */
>  
>  struct xcb_connection_t {
> +    /* This must be the first field; see _xcb_conn_ret_error(). */
>      int has_error;
>  
>      /* constant data */
>
On 18.01.2014 16:10, Ran Benita wrote:
> If xcb_connect() fails, it doesn't return NULL. Instead, it always
> returns an xcb_connection_t*, and the user should check for errors with
> the xcb_connection_has_error() function. What this function does is
> check if conn->has_error contains a non-zero error code, and returns it.
> 
> If an error did occur, xcb doesn't actually return a full
> xcb_connection_t though, it just returns (xcb_connection_t *)
> error_code. Since the 'has_error' field is the first, it is still
> possible to check conn->has_error.
> 
> That last trick was not immediately obvious to me, so add some guiding
> comments. This also ensures no one obliviously rearranges the struct.
> 
> Signed-off-by: Ran Benita <ran234@gmail.com>

Time for complaints is up and congratulation, this patch was selected for
today's "merge some random patch from patchwork":

    d7eb0bd..4ffa6f8  master -> master

Uli

> ---
>  src/xcb_conn.c | 3 +++
>  src/xcbint.h   | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/src/xcb_conn.c b/src/xcb_conn.c
> index 46390e1..00c458f 100644
> --- a/src/xcb_conn.c
> +++ b/src/xcb_conn.c
> @@ -374,6 +374,9 @@ void _xcb_conn_shutdown(xcb_connection_t *c, int err)
>  /* Return connection error state.
>   * To make thread-safe, I need a seperate static
>   * variable for every possible error.
> + * has_error is the first field in xcb_connection_t, so just
> + * return a casted int here; checking has_error (and only
> + * has_error) will be safe.
>   */
>  xcb_connection_t *_xcb_conn_ret_error(int err)
>  {
> diff --git a/src/xcbint.h b/src/xcbint.h
> index b25f03b..67cf571 100644
> --- a/src/xcbint.h
> +++ b/src/xcbint.h
> @@ -192,6 +192,7 @@ void _xcb_ext_destroy(xcb_connection_t *c);
>  /* xcb_conn.c */
>  
>  struct xcb_connection_t {
> +    /* This must be the first field; see _xcb_conn_ret_error(). */
>      int has_error;
>  
>      /* constant data */
>