| Message ID | 1394657145-2638-2-git-send-email-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > Current handling relies on atoi which does not detect errors > additionally, any integer value will be considered as a valid > percent. > > Resolve that by using strtol and limiting the value within > the 5-100 (percent) range. > > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> > --- > nouveau/nouveau.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c > index d6013db..06cd804 100644 > --- a/nouveau/nouveau.c > +++ b/nouveau/nouveau.c > @@ -76,7 +76,7 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) > struct nouveau_device *dev = &nvdev->base; > uint64_t chipset, vram, gart, bousage; > drmVersionPtr ver; > - int ret; > + int ret, limit; > char *tmp; > > #ifdef DEBUG > @@ -121,16 +121,22 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) > > nvdev->close = close; > > + nvdev->vram_limit_percent = 80; > tmp = getenv("NOUVEAU_LIBDRM_VRAM_LIMIT_PERCENT"); > - if (tmp) > - nvdev->vram_limit_percent = atoi(tmp); > - else > - nvdev->vram_limit_percent = 80; > + if (tmp) { > + limit = strtol(tmp, NULL, 10); > + if (limit >= 5 && limit <= 100) > + nvdev->vram_limit_percent = limit; > + } Wouldn't it be easier to just clamp the value no matter what? i.e. leave the current code alone, and add a clamp helper function + use it? These are pretty rare env vars to set... presumably the person setting them knows what they're doing. And if not, they get what they deserve. On a related topic, I'd personally rather not throw in arbitrary restrictions to code like this -- it makes debugging harder later on. (e.g. what's wrong with 0 percent? let's say i want to disable gart/vram entirely?) > + > + nvdev->gart_limit_percent = 80; > tmp = getenv("NOUVEAU_LIBDRM_GART_LIMIT_PERCENT"); > - if (tmp) > - nvdev->gart_limit_percent = atoi(tmp); > - else > - nvdev->gart_limit_percent = 80; > + if (tmp) { > + limit = strtol(tmp, NULL, 10); > + if (limit >= 5 && limit <= 100) > + nvdev->gart_limit_percent = limit; > + } > + > DRMINITLISTHEAD(&nvdev->bo_list); > nvdev->base.object.oclass = NOUVEAU_DEVICE_CLASS; > nvdev->base.lib_version = 0x01000000; > -- > 1.9.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 13/03/14 00:49, Ilia Mirkin wrote: > On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> Current handling relies on atoi which does not detect errors >> additionally, any integer value will be considered as a valid >> percent. >> >> Resolve that by using strtol and limiting the value within >> the 5-100 (percent) range. >> >> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> >> --- >> nouveau/nouveau.c | 24 +++++++++++++++--------- >> 1 file changed, 15 insertions(+), 9 deletions(-) >> >> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c >> index d6013db..06cd804 100644 >> --- a/nouveau/nouveau.c >> +++ b/nouveau/nouveau.c >> @@ -76,7 +76,7 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) >> struct nouveau_device *dev = &nvdev->base; >> uint64_t chipset, vram, gart, bousage; >> drmVersionPtr ver; >> - int ret; >> + int ret, limit; >> char *tmp; >> >> #ifdef DEBUG >> @@ -121,16 +121,22 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) >> >> nvdev->close = close; >> >> + nvdev->vram_limit_percent = 80; >> tmp = getenv("NOUVEAU_LIBDRM_VRAM_LIMIT_PERCENT"); >> - if (tmp) >> - nvdev->vram_limit_percent = atoi(tmp); >> - else >> - nvdev->vram_limit_percent = 80; >> + if (tmp) { >> + limit = strtol(tmp, NULL, 10); >> + if (limit >= 5 && limit <= 100) >> + nvdev->vram_limit_percent = limit; >> + } > > Wouldn't it be easier to just clamp the value no matter what? i.e. > leave the current code alone, and add a clamp helper function + use > it? These are pretty rare env vars to set... presumably the person > setting them knows what they're doing. And if not, they get what they > deserve. > > On a related topic, I'd personally rather not throw in arbitrary > restrictions to code like this -- it makes debugging harder later on. > (e.g. what's wrong with 0 percent? let's say i want to disable > gart/vram entirely?) > Valid arguments, thanks. -Emil >> + >> + nvdev->gart_limit_percent = 80; >> tmp = getenv("NOUVEAU_LIBDRM_GART_LIMIT_PERCENT"); >> - if (tmp) >> - nvdev->gart_limit_percent = atoi(tmp); >> - else >> - nvdev->gart_limit_percent = 80; >> + if (tmp) { >> + limit = strtol(tmp, NULL, 10); >> + if (limit >= 5 && limit <= 100) >> + nvdev->gart_limit_percent = limit; >> + } >> + >> DRMINITLISTHEAD(&nvdev->bo_list); >> nvdev->base.object.oclass = NOUVEAU_DEVICE_CLASS; >> nvdev->base.lib_version = 0x01000000; >> -- >> 1.9.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c index d6013db..06cd804 100644 --- a/nouveau/nouveau.c +++ b/nouveau/nouveau.c @@ -76,7 +76,7 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) struct nouveau_device *dev = &nvdev->base; uint64_t chipset, vram, gart, bousage; drmVersionPtr ver; - int ret; + int ret, limit; char *tmp; #ifdef DEBUG @@ -121,16 +121,22 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) nvdev->close = close; + nvdev->vram_limit_percent = 80; tmp = getenv("NOUVEAU_LIBDRM_VRAM_LIMIT_PERCENT"); - if (tmp) - nvdev->vram_limit_percent = atoi(tmp); - else - nvdev->vram_limit_percent = 80; + if (tmp) { + limit = strtol(tmp, NULL, 10); + if (limit >= 5 && limit <= 100) + nvdev->vram_limit_percent = limit; + } + + nvdev->gart_limit_percent = 80; tmp = getenv("NOUVEAU_LIBDRM_GART_LIMIT_PERCENT"); - if (tmp) - nvdev->gart_limit_percent = atoi(tmp); - else - nvdev->gart_limit_percent = 80; + if (tmp) { + limit = strtol(tmp, NULL, 10); + if (limit >= 5 && limit <= 100) + nvdev->gart_limit_percent = limit; + } + DRMINITLISTHEAD(&nvdev->bo_list); nvdev->base.object.oclass = NOUVEAU_DEVICE_CLASS; nvdev->base.lib_version = 0x01000000;
Current handling relies on atoi which does not detect errors additionally, any integer value will be considered as a valid percent. Resolve that by using strtol and limiting the value within the 5-100 (percent) range. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> --- nouveau/nouveau.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)