From mboxrd@z Thu Jan 1 00:00:00 1970 From: Emil Velikov Subject: Re: [PATCH 1/3] nouveau: cleanup error handling during nouveau_device_wrap Date: Thu, 13 Mar 2014 01:04:40 +0000 Message-ID: <53210428.3030209@gmail.com> References: <1394657145-2638-1-git-send-email-emil.l.velikov@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f43.google.com (mail-wg0-f43.google.com [74.125.82.43]) by gabe.freedesktop.org (Postfix) with ESMTP id 4FBA7FA2D0 for ; Wed, 12 Mar 2014 18:02:57 -0700 (PDT) Received: by mail-wg0-f43.google.com with SMTP id x13so236908wgg.26 for ; Wed, 12 Mar 2014 18:02:56 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Ilia Mirkin Cc: emil.l.velikov@gmail.com, "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org On 13/03/14 00:45, Ilia Mirkin wrote: > On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov 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 >> --- >> 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