[Mesa-dev] [PATCH v2 1/2] egl/dri2: Close file descriptor on error.

[Emil Velikov]

On 9 September 2015 at 01:43, Ian Romanick <idr at freedesktop.org> wrote:
> On 09/07/2015 01:58 AM, Emil Velikov wrote:
>> From: Matt Turner <mattst88 at gmail.com>
>>
>> v2: [Emil Velikov]
>> Rework the error path to a common goto, close only if we own the fd.
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>> ---
>>  src/egl/drivers/dri2/platform_drm.c | 27 ++++++++++++++-------------
>>  1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
>> index eda5087..e8fe7ea 100644
>> --- a/src/egl/drivers/dri2/platform_drm.c
>> +++ b/src/egl/drivers/dri2/platform_drm.c
>> @@ -623,26 +623,20 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>>        dri2_dpy->own_device = 1;
>>        gbm = gbm_create_device(fd);
>>        if (gbm == NULL)
>> -         return EGL_FALSE;
>> +         goto cleanup;
>>     }
>>
>> -   if (strcmp(gbm_device_get_backend_name(gbm), "drm") != 0) {
>> -      free(dri2_dpy);
>> -      return EGL_FALSE;
>> -   }
>> +   if (strcmp(gbm_device_get_backend_name(gbm), "drm") != 0)
>> +      goto cleanup;
>>
>>     dri2_dpy->gbm_dri = gbm_dri_device(gbm);
>> -   if (dri2_dpy->gbm_dri->base.type != GBM_DRM_DRIVER_TYPE_DRI) {
>> -      free(dri2_dpy);
>> -      return EGL_FALSE;
>> -   }
>> +   if (dri2_dpy->gbm_dri->base.type != GBM_DRM_DRIVER_TYPE_DRI)
>> +      goto cleanup;
>>
>>     if (fd < 0) {
>>        fd = fcntl(gbm_device_get_fd(gbm), F_DUPFD_CLOEXEC, 3);
>> -      if (fd < 0) {
>> -         free(dri2_dpy);
>> -         return EGL_FALSE;
>> -      }
>> +      if (fd < 0)
>> +         goto cleanup;
>
> Shouldn't this fd also get closed eventually?  If we decide to catch
> other failures (e.g., memory allocation failures) later in this function?
>
By 'this fd' I assume you mean the dup'd fd as opposed to the
device_fd ? Yes we should, Matt's patch was doing the correct thing as
I misread the DUPFD part in F_DUPFD_CLOEXEC :-\

Thanks
Emil