diff mbox

[libdrm,v2,04/10] xf86drm: Allocate drmDevicePtr's on stack

Message ID 20180629152017.29843-1-emil.l.velikov@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Emil Velikov June 29, 2018, 3:20 p.m. UTC 
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.

v2:
 - add a define & description behind the magic 256 (Rob)
 - report error to strerr and skip when over 256 device nodes (Rob)

Cc: Robert Foss <robert.foss@collabora.com>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
Tested-by: Robert Foss <robert.foss@collabora.com> (v1)
Reviewed-by: Robert Foss <robert.foss@collabora.com> (v1)
Reviewed-by: Eric Engestrom <eric@engestrom.ch> (v1)
---
 xf86drm.c | 79 ++++++++++++++++---------------------------------------
 1 file changed, 23 insertions(+), 56 deletions(-)

Comments

Robert Foss June 29, 2018, 3:49 p.m. UTC | #1 
LGTM

On 2018-06-29 17:20, 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.
> 
> v2:
>   - add a define & description behind the magic 256 (Rob)
>   - report error to strerr and skip when over 256 device nodes (Rob)
> 
> Cc: Robert Foss <robert.foss@collabora.com>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> Tested-by: Robert Foss <robert.foss@collabora.com> (v1)
> Reviewed-by: Robert Foss <robert.foss@collabora.com> (v1)
> Reviewed-by: Eric Engestrom <eric@engestrom.ch> (v1)
> ---
>   xf86drm.c | 79 ++++++++++++++++---------------------------------------
>   1 file changed, 23 insertions(+), 56 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 114cf855..02da3e1f 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3766,6 +3766,13 @@ drm_device_has_rdev(drmDevicePtr device, dev_t find_rdev)
>       return false;
>   }
>   
> +/*
> + * 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. Rounded it up to 256 for simplicity.
> + */
> +#define MAX_DRM_NODES 256
> +
>   /**
>    * Get information about the opened drm device
>    *
> @@ -3846,7 +3853,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>   
>       return 0;
>   #else
> -    drmDevicePtr *local_devices;
> +    drmDevicePtr local_devices[MAX_DRM_NODES];
>       drmDevicePtr d;
>       DIR *sysdir;
>       struct dirent *dent;
> @@ -3854,7 +3861,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 +3883,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 +3893,12 @@ 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;
> +        if (i >= MAX_DRM_NODES) {
> +            fprintf(stderr, "More than %d drm nodes detected. "
> +                    "Please report a bug - that should not happen.\n"
> +                    "Skipping extra nodes\n", MAX_DRM_NODES);
> +            break;
>           }
> -
>           local_devices[i] = d;
>           i++;
>       }
> @@ -3921,18 +3917,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 +3955,18 @@ int drmGetDevice(int fd, drmDevicePtr *device)
>    */
>   int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
>   {
> -    drmDevicePtr *local_devices;
> +    drmDevicePtr local_devices[MAX_DRM_NODES];
>       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 +3974,12 @@ 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;
> +        if (i >= MAX_DRM_NODES) {
> +            fprintf(stderr, "More than %d drm nodes detected. "
> +                    "Please report a bug - that should not happen.\n"
> +                    "Skipping extra nodes\n", MAX_DRM_NODES);
> +            break;
>           }
> -
>           local_devices[i] = device;
>           i++;
>       }
> @@ -4025,16 +4001,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;
>   }
>   
>   /**
>
diff mbox

Patch

diff --git a/xf86drm.c b/xf86drm.c
index 114cf855..02da3e1f 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3766,6 +3766,13 @@  drm_device_has_rdev(drmDevicePtr device, dev_t find_rdev)
     return false;
 }
 
+/*
+ * 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. Rounded it up to 256 for simplicity.
+ */
+#define MAX_DRM_NODES 256
+
 /**
  * Get information about the opened drm device
  *
@@ -3846,7 +3853,7 @@  int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
 
     return 0;
 #else
-    drmDevicePtr *local_devices;
+    drmDevicePtr local_devices[MAX_DRM_NODES];
     drmDevicePtr d;
     DIR *sysdir;
     struct dirent *dent;
@@ -3854,7 +3861,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 +3883,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 +3893,12 @@  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;
+        if (i >= MAX_DRM_NODES) {
+            fprintf(stderr, "More than %d drm nodes detected. "
+                    "Please report a bug - that should not happen.\n"
+                    "Skipping extra nodes\n", MAX_DRM_NODES);
+            break;
         }
-
         local_devices[i] = d;
         i++;
     }
@@ -3921,18 +3917,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 +3955,18 @@  int drmGetDevice(int fd, drmDevicePtr *device)
  */
 int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
 {
-    drmDevicePtr *local_devices;
+    drmDevicePtr local_devices[MAX_DRM_NODES];
     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 +3974,12 @@  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;
+        if (i >= MAX_DRM_NODES) {
+            fprintf(stderr, "More than %d drm nodes detected. "
+                    "Please report a bug - that should not happen.\n"
+                    "Skipping extra nodes\n", MAX_DRM_NODES);
+            break;
         }
-
         local_devices[i] = device;
         i++;
     }
@@ -4025,16 +4001,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;
 }
 
 /**