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

[Emil Velikov <emil.l.velikov@gmail.com>]

On 13/03/14 00:45, Ilia Mirkin wrote:
> On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov@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.

>
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel