Re: [PATCH libdrm 04/10] xf86drm: Allocate drmDevicePtr's on stack

[Robert Foss <robert.foss@collabora.com>]

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;
>   }
>   
>   /**
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel