[Mesa-dev] [PATCH 01/10] gbm: Set errno on errors

[Emil Velikov]

On 04/04/14 09:36, Ander Conselvan de Oliveira wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> 
> This should give the caller some information of what called the error.
> For the gbm_bo_import() case, for instance, it is possible to know if
> the import is not supported or the error was caused by an invalid
> parameter.
Perhaps I'm seeing things but you're setting errno to EINVAL in almost all
cases, which IMHO is not that much of an improvement. A couple of suggestions
below and I'm assuming that some of the rest can be more descriptive as well.

Browsing through the file I've noticed that there are a few cases where errno
is not set. Was that intentional (example when we fail to allocate memory) ?

> ---
>  src/gbm/backends/dri/gbm_dri.c | 38 ++++++++++++++++++++++++++++++--------
>  src/gbm/main/gbm.c             | 20 +++++++++++++-------
>  2 files changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c
> index 50fa588..9d08a97 100644
> --- a/src/gbm/backends/dri/gbm_dri.c
> +++ b/src/gbm/backends/dri/gbm_dri.c
> @@ -30,6 +30,7 @@
>  #include <stddef.h>
>  #include <stdint.h>
>  #include <string.h>
> +#include <errno.h>
>  #include <limits.h>
>  
>  #include <sys/types.h>
> @@ -353,8 +354,10 @@ gbm_dri_bo_write(struct gbm_bo *_bo, const void *buf, size_t count)
>  {
>     struct gbm_dri_bo *bo = gbm_dri_bo(_bo);
>  
> -   if (bo->image != NULL)
> +   if (bo->image != NULL) {
> +      errno = EINVAL;
The gbm device does not have the image DRI extension so should we be using
ENOSYS ?

>        return -1;
> +   }
>  
>     memcpy(bo->map, buf, count);
>  
> @@ -432,8 +435,10 @@ gbm_dri_bo_import(struct gbm_device *gbm,
>     int gbm_format;
>  
>     /* Required for query image WIDTH & HEIGHT */
> -   if (dri->image->base.version < 4)
> +   if (dri->image->base.version < 4) {
> +      errno = ENOSYS;
>        return NULL;
> +   }
>  
>     switch (type) {
>  #if HAVE_WAYLAND_PLATFORM
> @@ -441,12 +446,16 @@ gbm_dri_bo_import(struct gbm_device *gbm,
>     {
>        struct wl_drm_buffer *wb;
>  
> -      if (!dri->wl_drm)
> +      if (!dri->wl_drm) {
> +         errno = EINVAL;
>           return NULL;
> +      }
>  
>        wb = wayland_drm_buffer_get(dri->wl_drm, (struct wl_resource *) buffer);
> -      if (!wb)
> +      if (!wb) {
> +         errno = EINVAL;
>           return NULL;
> +      }
>  
>        image = dri->image->dupImage(wb->driver_buffer, NULL);
>  
> @@ -473,15 +482,19 @@ gbm_dri_bo_import(struct gbm_device *gbm,
>     case GBM_BO_IMPORT_EGL_IMAGE:
>     {
>        int dri_format;
> -      if (dri->lookup_image == NULL)
> +      if (dri->lookup_image == NULL) {
> +         errno = EINVAL;
>           return NULL;
> +      }
>  
>        image = dri->lookup_image(dri->screen, buffer, dri->lookup_user_data);
>        image = dri->image->dupImage(image, NULL);
>        dri->image->queryImage(image, __DRI_IMAGE_ATTRIB_FORMAT, &dri_format);
>        gbm_format = gbm_dri_to_gbm_format(dri_format);
> -      if (gbm_format == 0)
> +      if (gbm_format == 0) {
> +         errno = EINVAL;
>           return NULL;
> +      }
>        break;
>     }
>  
> @@ -502,6 +515,7 @@ gbm_dri_bo_import(struct gbm_device *gbm,
>     }
>  
>     default:
> +      errno = ENOSYS;
>        return NULL;
>     }
>  
> @@ -518,6 +532,7 @@ gbm_dri_bo_import(struct gbm_device *gbm,
>        dri_use |= __DRI_IMAGE_USE_CURSOR;
>     if (dri->image->base.version >= 2 &&
>         !dri->image->validateUsage(bo->image, dri_use)) {
> +      errno = EINVAL;
ENOSYS ?

>        free(bo);
>        return NULL;
>     }
> @@ -549,10 +564,14 @@ create_dumb(struct gbm_device *gbm,
>     struct drm_mode_destroy_dumb destroy_arg;
>     int ret;
>  
> -   if (!(usage & GBM_BO_USE_CURSOR_64X64))
> +   if (!(usage & GBM_BO_USE_CURSOR_64X64)) {
> +      errno = EINVAL;
>        return NULL;
> -   if (format != GBM_FORMAT_ARGB8888)
> +   }
> +   if (format != GBM_FORMAT_ARGB8888) {
> +      errno = EINVAL;
>        return NULL;
> +   }
>  
>     bo = calloc(1, sizeof *bo);
>     if (bo == NULL)
> @@ -643,6 +662,7 @@ gbm_dri_bo_create(struct gbm_device *gbm,
>        dri_format = __DRI_IMAGE_FORMAT_XRGB2101010;
>        break;
>     default:
> +      errno = EINVAL;
>        goto failed;
>     }
>  
> @@ -722,6 +742,8 @@ dri_device_create(int fd)
>     int ret;
>  
>     dri = calloc(1, sizeof *dri);
> +   if (!dri)
> +      return NULL;
>  
IMHO this hunk can to in a separate patch and queued for the stable branches.

Cheers
-Emil