Bug 236436

Created attachment 451514 [details] Patch

Comment on attachment 451514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451514&action=review Much better! However, this currently includes an invalid memory write error - that needs to be addressed before commit and I think the other comment should also be addressed to avoid possible errors in future. You can assume my r+ with those fixed (but feel free to ping me for review if you aren't in a rush :)) > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:74 > + memset(devices, 0, sizeof(drmDevicePtr) * 64); This * 64 is incorrect, sizeof(drmDevicePtr) should be correct on its own. > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:76 > + int numDevices = drmGetDevices2(0, devices, 64); Rather than putting 64 here, it would be good to use an array length macro (something of the form `#define ARRAY_LENGTH(x) (sizeof(x) / sizeof((x)[0]))` - I expect we have something like this already in WTF?)

Thanks for the feedbak! (In reply to Chris Lord from comment #2) > Comment on attachment 451514 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451514&action=review > > Much better! However, this currently includes an invalid memory write error > - that needs to be addressed before commit and I think the other comment > should also be addressed to avoid possible errors in future. You can assume > my r+ with those fixed (but feel free to ping me for review if you aren't in > a rush :)) > > > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:74 > > + memset(devices, 0, sizeof(drmDevicePtr) * 64); > > This * 64 is incorrect, sizeof(drmDevicePtr) should be correct on its own. > Oops, yep :-). > > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:76 > > + int numDevices = drmGetDevices2(0, devices, 64); > > Rather than putting 64 here, it would be good to use an array length macro > (something of the form `#define ARRAY_LENGTH(x) (sizeof(x) / > sizeof((x)[0]))` - I expect we have something like this already in WTF?) Ok, I'll try to find something Thanks for the review!

Comment on attachment 451514 [details] Patch > > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:74 > > + memset(devices, 0, sizeof(drmDevicePtr) * 64); > > This * 64 is incorrect, sizeof(drmDevicePtr) should be correct on its own. Whoops, we just spoke about this and I wanted to add here - I misread this and what's there is fine, but `sizeof(devices)` along with the other suggested change would make this patch more robust. Changing my r- to an r+ to reflect that mistake.

Created attachment 451569 [details] Patch for landing

Committed r289575 (247092@main): <https://commits.webkit.org/247092@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451569 [details].

<rdar://problem/88777827>

Comment on attachment 451569 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=451569&action=review > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:58 > + int fd = getDeviceFd(); This file descriptor, when valid, gets leaked. gbm_device doesn't own it. > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:71 > +int GBMDevice::getDeviceFd() This should be a static method. Better yet, if GBMDevice is kept as thread-specific, this file descriptor could instead be global, and the drmGetDevices2 lookup would then only be done once, not for every initialization.

(In reply to Zan Dobersek from comment #8) > Comment on attachment 451569 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451569&action=review > > > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:58 > > + int fd = getDeviceFd(); > > This file descriptor, when valid, gets leaked. gbm_device doesn't own it. > > > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:71 > > +int GBMDevice::getDeviceFd() > > This should be a static method. Better yet, if GBMDevice is kept as > thread-specific, this file descriptor could instead be global, and the > drmGetDevices2 lookup would then only be done once, not for every > initialization. Thanks for the comments! I'll address this and some other proposals about the drm device to choose in a following patch! :-)