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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



LGTM

On 2018-06-29 17:20, Emil Velikov wrote:
From: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>

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@xxxxxxxxxxxxx>
Signed-off-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
Tested-by: Robert Foss <robert.foss@xxxxxxxxxxxxx> (v1)
Reviewed-by: Robert Foss <robert.foss@xxxxxxxxxxxxx> (v1)
Reviewed-by: Eric Engestrom <eric@xxxxxxxxxxxx> (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;
  }
/**

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux