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

[Emil Velikov <emil.l.velikov@gmail.com>]

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