| Message ID | 20180625173626.8046-4-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hey Emil, On 2018-06-25 19:36, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > Currently we dynamically allocate 16 pointers and reallocate more as > needed. > > Instead, allocate the maximum number (256) on stack - the number is > small enough and is unlikely to change in the foreseeable future. > > This allows us to simplify the error handling and even shed a few bytes > off the final binary. > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > --- > xf86drm.c | 64 ++++++------------------------------------------------- > 1 file changed, 6 insertions(+), 58 deletions(-) > > diff --git a/xf86drm.c b/xf86drm.c > index 114cf855..d4810740 100644 > --- a/xf86drm.c > +++ b/xf86drm.c > @@ -3846,7 +3846,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) > > return 0; > #else > - drmDevicePtr *local_devices; > + drmDevicePtr local_devices[256]; This number is seen later on in this patch, maybe it should be broken out into a define, since it's reused later on too at [1]. > drmDevicePtr d; > DIR *sysdir; > struct dirent *dent; > @@ -3854,7 +3854,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) > int subsystem_type; > int maj, min; > int ret, i, node_count; > - int max_count = 16; > dev_t find_rdev; > > if (drm_device_validate_flags(flags)) > @@ -3877,15 +3876,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) > if (subsystem_type < 0) > return subsystem_type; > > - local_devices = calloc(max_count, sizeof(drmDevicePtr)); > - if (local_devices == NULL) > - return -ENOMEM; > - > sysdir = opendir(DRM_DIR_NAME); > - if (!sysdir) { > - ret = -errno; > - goto free_locals; > - } > + if (!sysdir) > + return -errno; > > i = 0; > while ((dent = readdir(sysdir))) { > @@ -3893,16 +3886,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) > if (ret) > continue; > > - if (i >= max_count) { Is this check really not needded what exactly is it that defines 256 as the maximum? From what I understand readdir(sysdir) is the call that defines how many devices will be looked through, and as far as I understand it can return an arbitrary number of files. > - drmDevicePtr *temp; > - > - max_count += 16; > - temp = realloc(local_devices, max_count * sizeof(drmDevicePtr)); > - if (!temp) > - goto free_devices; > - local_devices = temp; > - } > - > local_devices[i] = d; > i++; > } > @@ -3921,18 +3904,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) > } > > closedir(sysdir); > - free(local_devices); > if (*device == NULL) > return -ENODEV; > return 0; > - > -free_devices: > - drmFreeDevices(local_devices, i); > - closedir(sysdir); > - > -free_locals: > - free(local_devices); > - return ret; > #endif > } > > @@ -3968,25 +3942,18 @@ int drmGetDevice(int fd, drmDevicePtr *device) > */ > int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) > { > - drmDevicePtr *local_devices; > + drmDevicePtr local_devices[256];' [1] > drmDevicePtr device; > DIR *sysdir; > struct dirent *dent; > int ret, i, node_count, device_count; > - int max_count = 16; > > if (drm_device_validate_flags(flags)) > return -EINVAL; > > - local_devices = calloc(max_count, sizeof(drmDevicePtr)); > - if (local_devices == NULL) > - return -ENOMEM; > - > sysdir = opendir(DRM_DIR_NAME); > - if (!sysdir) { > - ret = -errno; > - goto free_locals; > - } > + if (!sysdir) > + return -errno; > > i = 0; > while ((dent = readdir(sysdir))) { > @@ -3994,16 +3961,6 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) > if (ret) > continue; > > - if (i >= max_count) { This out of bounds check should be dealt with too, if the above bounds check should be. > - drmDevicePtr *temp; > - > - max_count += 16; > - temp = realloc(local_devices, max_count * sizeof(drmDevicePtr)); > - if (!temp) > - goto free_devices; > - local_devices = temp; > - } > - > local_devices[i] = device; > i++; > } > @@ -4025,16 +3982,7 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) > } > > closedir(sysdir); > - free(local_devices); > return device_count; > - > -free_devices: > - drmFreeDevices(local_devices, i); > - closedir(sysdir); > - > -free_locals: > - free(local_devices); > - return ret; > } > > /** >
Hi Rob, On 28 June 2018 at 13:52, Robert Foss <robert.foss@collabora.com> wrote: > Hey Emil, > > On 2018-06-25 19:36, Emil Velikov wrote: >> >> From: Emil Velikov <emil.velikov@collabora.com> >> >> Currently we dynamically allocate 16 pointers and reallocate more as >> needed. >> >> Instead, allocate the maximum number (256) on stack - the number is >> small enough and is unlikely to change in the foreseeable future. >> >> This allows us to simplify the error handling and even shed a few bytes >> off the final binary. >> >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> >> --- >> xf86drm.c | 64 ++++++------------------------------------------------- >> 1 file changed, 6 insertions(+), 58 deletions(-) >> >> diff --git a/xf86drm.c b/xf86drm.c >> index 114cf855..d4810740 100644 >> --- a/xf86drm.c >> +++ b/xf86drm.c >> @@ -3846,7 +3846,7 @@ int drmGetDevice2(int fd, uint32_t flags, >> drmDevicePtr *device) >> return 0; >> #else >> - drmDevicePtr *local_devices; >> + drmDevicePtr local_devices[256]; > > > This number is seen later on in this patch, maybe it should be broken out > into a > define, since it's reused later on too at [1]. > Sure thing. The lame MAX_DRM_NODES comes to mind, so alternatives are welcome. > >> drmDevicePtr d; >> DIR *sysdir; >> struct dirent *dent; >> @@ -3854,7 +3854,6 @@ int drmGetDevice2(int fd, uint32_t flags, >> drmDevicePtr *device) >> int subsystem_type; >> int maj, min; >> int ret, i, node_count; >> - int max_count = 16; >> dev_t find_rdev; >> if (drm_device_validate_flags(flags)) >> @@ -3877,15 +3876,9 @@ int drmGetDevice2(int fd, uint32_t flags, >> drmDevicePtr *device) >> if (subsystem_type < 0) >> return subsystem_type; >> - local_devices = calloc(max_count, sizeof(drmDevicePtr)); >> - if (local_devices == NULL) >> - return -ENOMEM; >> - >> sysdir = opendir(DRM_DIR_NAME); >> - if (!sysdir) { >> - ret = -errno; >> - goto free_locals; >> - } >> + if (!sysdir) >> + return -errno; >> i = 0; >> while ((dent = readdir(sysdir))) { >> @@ -3893,16 +3886,6 @@ int drmGetDevice2(int fd, uint32_t flags, >> drmDevicePtr *device) >> if (ret) >> continue; >> - if (i >= max_count) { > > > Is this check really not needded what exactly is it that defines 256 as the > maximum? > > From what I understand readdir(sysdir) is the call that defines how many > devices will be looked through, and as far as I understand it can return an > arbitrary number of files. > The kernel drm core has a number of places that assume maximum of 3x64 devices nodes. That's 64 for each of primary, control and render nodes. I've rounded it up to 256 for simplicity. I assume we'll be able to see if anyone changes the kernel, although bailing out just in case is a good idea. The is one exciting error message - let me know if anything else comes to mind. if (i >= MAX_DRM_NODES) { fprintf(stderr, "More than %d drm nodes detected. Please report a bug - that should not happen.\nSkipping extra nodes\n", MAX_DRM_NODES); break; } I'll send out v2 for the patches that need work some time tomorrow/over the weekend.. Thanks Emil
diff --git a/xf86drm.c b/xf86drm.c index 114cf855..d4810740 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -3846,7 +3846,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) return 0; #else - drmDevicePtr *local_devices; + drmDevicePtr local_devices[256]; drmDevicePtr d; DIR *sysdir; struct dirent *dent; @@ -3854,7 +3854,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) int subsystem_type; int maj, min; int ret, i, node_count; - int max_count = 16; dev_t find_rdev; if (drm_device_validate_flags(flags)) @@ -3877,15 +3876,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) if (subsystem_type < 0) return subsystem_type; - local_devices = calloc(max_count, sizeof(drmDevicePtr)); - if (local_devices == NULL) - return -ENOMEM; - sysdir = opendir(DRM_DIR_NAME); - if (!sysdir) { - ret = -errno; - goto free_locals; - } + if (!sysdir) + return -errno; i = 0; while ((dent = readdir(sysdir))) { @@ -3893,16 +3886,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) if (ret) continue; - if (i >= max_count) { - drmDevicePtr *temp; - - max_count += 16; - temp = realloc(local_devices, max_count * sizeof(drmDevicePtr)); - if (!temp) - goto free_devices; - local_devices = temp; - } - local_devices[i] = d; i++; } @@ -3921,18 +3904,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) } closedir(sysdir); - free(local_devices); if (*device == NULL) return -ENODEV; return 0; - -free_devices: - drmFreeDevices(local_devices, i); - closedir(sysdir); - -free_locals: - free(local_devices); - return ret; #endif } @@ -3968,25 +3942,18 @@ int drmGetDevice(int fd, drmDevicePtr *device) */ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) { - drmDevicePtr *local_devices; + drmDevicePtr local_devices[256]; drmDevicePtr device; DIR *sysdir; struct dirent *dent; int ret, i, node_count, device_count; - int max_count = 16; if (drm_device_validate_flags(flags)) return -EINVAL; - local_devices = calloc(max_count, sizeof(drmDevicePtr)); - if (local_devices == NULL) - return -ENOMEM; - sysdir = opendir(DRM_DIR_NAME); - if (!sysdir) { - ret = -errno; - goto free_locals; - } + if (!sysdir) + return -errno; i = 0; while ((dent = readdir(sysdir))) { @@ -3994,16 +3961,6 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) if (ret) continue; - if (i >= max_count) { - drmDevicePtr *temp; - - max_count += 16; - temp = realloc(local_devices, max_count * sizeof(drmDevicePtr)); - if (!temp) - goto free_devices; - local_devices = temp; - } - local_devices[i] = device; i++; } @@ -4025,16 +3982,7 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) } closedir(sysdir); - free(local_devices); return device_count; - -free_devices: - drmFreeDevices(local_devices, i); - closedir(sysdir); - -free_locals: - free(local_devices); - return ret; } /**