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

[Ian Romanick]

On 09/09/2015 10:59 AM, Emil Velikov wrote:
> 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 :-\

Yeah, that's what I meant.

In spite of that, I like the 'goto cleanup' change.  I was going to
suggest that to Matt... but you went and did it before I could suggest
it. :)  Maybe blend the strengths?

> Thanks
> Emil