On 04/04/14 09:36, Ander Conselvan de Oliveira wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.olive...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
