[PATCH 1/3] nouveau: cleanup error handling during nouveau_device_wrap

[Ilia Mirkin]

On Wed, Mar 12, 2014 at 9:04 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 13/03/14 00:45, Ilia Mirkin wrote:
>>
>> On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov at gmail.com>
>> wrote:
>>>
>>> In theory it's possible for any of the nouveau_getparam calls to
>>> fail whist the last one being successful.
>>>
>>> Thus at least one of the following (hard requirements) drmVersion,
>>> chipset and vram/gart memory size will be filled with garbage and
>>> sent to the userspace drivers.
>>
>>
>> What was wrong with the old logic again? Except annoying indentation?
>>
>> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
>> if (ret == 0)
>> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
>> if (ret == 0)
>> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
>> if (ret) {
>> nouveau_device_del(&dev);
>> return ret;
>> }
>>
>> It will only run each successive getparam if the previous one succeeded...
>>
> Good point, the lovely indentation got me. So it seems only !ver handling is
> needed.

Not really. drm->drm_version will be 0 if ver fails.

>
>
>>
>>>
>>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>>> ---
>>>   nouveau/nouveau.c | 30 +++++++++++++++++++-----------
>>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>>> index ee7893b..d6013db 100644
>>> --- a/nouveau/nouveau.c
>>> +++ b/nouveau/nouveau.c
>>> @@ -88,27 +88,32 @@ nouveau_device_wrap(int fd, int close, struct
>>> nouveau_device **pdev)
>>>          nvdev->base.fd = fd;
>>>
>>>          ver = drmGetVersion(fd);
>>> -       if (ver) dev->drm_version = (ver->version_major << 24) |
>>> -                                   (ver->version_minor << 8) |
>>> -                                    ver->version_patchlevel;
>>> +       if (!ver) {
>>> +               ret = -errno;
>>> +               goto error;
>>> +       }
>>> +
>>> +       dev->drm_version = (ver->version_major << 24) |
>>> +                           (ver->version_minor << 8) |
>>> +                            ver->version_patchlevel;
>>>          drmFreeVersion(ver);
>>>
>>>          if ( dev->drm_version != 0x00000010 &&
>>>              (dev->drm_version <  0x01000000 ||
>>>               dev->drm_version >= 0x02000000)) {
>>> -               nouveau_device_del(&dev);
>>> -               return -EINVAL;
>>> +               ret = -EINVAL;
>>> +               goto error;
>>>          }
>>>
>>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID,
>>> &chipset);
>>> -       if (ret == 0)
>>> +       if (ret)
>>> +               goto error;
>>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
>>> -       if (ret == 0)
>>> +       if (ret)
>>> +               goto error;
>>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
>>> -       if (ret) {
>>> -               nouveau_device_del(&dev);
>>> -               return ret;
>>> -       }
>>> +       if (ret)
>>> +               goto error;
>>>
>>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_HAS_BO_USAGE,
>>> &bousage);
>>>          if (ret == 0)
>>> @@ -139,6 +144,9 @@ nouveau_device_wrap(int fd, int close, struct
>>> nouveau_device **pdev)
>>>
>>>          *pdev = &nvdev->base;
>>>          return 0;
>>> +error:
>>> +       nouveau_device_del(&dev);
>>> +       return -ret;
>>
>>
>> you mean 'ret' of course.
>>
> Yikes, thanks.
>
> -Emil
>
>
>>>   }
>>>
>>>   int
>>> --
>>> 1.9.0
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>