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

[Ander Conselvan de Oliveira]

On 04/04/2014 01:37 PM, Emil Velikov wrote:
> 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) ?

Yep, malloc() and calloc() set errno to ENOMEM.

See other comments below.

>
>> ---
>>   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 ?

Not having the image DRI extension is a fatal error for the gbm dri 
backend. However, the check above returns EINVAL if the bo supplied to 
gbm_bo_write() has a DRI image allocated for it. That is invalid because 
we only allow gbm_bo_write() for bos backed by dumb buffers (allocated 
with the GBM_BO_USE_WRITE flag) and those don't have a DRI image.


>>         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 ?

This is a tricky one. The patch doesn't change the behavior, which is 
try to validate the usage if the version of the DRI image extension 
implements validateUsage(), otherwise, just assume things will work. So 
the failure only happens if the call to validateUsage() fails, and that 
means the imported buffer is incompatible with the requested usage.

>>         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.

That makes sense. I'll split and resend.

Thanks for reviewing.

Cheers,
Ander