[PATCH] libpciaccess: close mtrr fd on pci_cleanup
jeremyhu at apple.com
Mon Oct 24 10:49:19 PDT 2011
The module which opens the fd should be the same module that closes it. Letting that cross between the common/specific boundary seems problematic. I'd prefer to see a new hook added for implementation-specific cleanup, and the close() live in linux_sysfs.c itself. That will allow for better abstraction down the road as other implementations might want to do something similar.
On Oct 24, 2011, at 10:18, Nithin Sujir wrote:
> On 10/24/2011 09:51 AM, Jeremy Huddleston wrote:
>> While possibly safe in practice, this doesn't look like the correct fix. It is possible that this will result in a close(0) if HAVE_MTRR is defined and mtrr_fd was just never set.
>> HAVE_MTRR is defined if<asm/mtrr.h> exists.
>> mtrr_fd is set if HAVE_MTRR is defined and linux_sysfs is used.
>> Probably a trivial example of this would be to "sudo touch /usr/include/asm/mtrr.h" on FreeBSD.
> That is a valid point. I don't have a freebsd system to test it but based on code review I agree that what you say will happen.
> Would you suggest adding an #ifdef linux around the close or since the pci_sys structure is allocated with a calloc either directly or indirectly, I can change the condition to check for > 0.
>> On Oct 21, 2011, at 11:49, Nithin Nayak Sujir wrote:
>>> Since the fd is not closed, calling pci_system_init and
>>> pci_system_cleanup more than 1024 times results in "too many files open"
>>> Signed-off-by: Nithin Nayak Sujir<nsujir at broadcom.com>
>>> src/common_init.c | 6 ++++++
>>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>> diff --git a/src/common_init.c b/src/common_init.c
>>> index 5e91c27..d7ade3f 100644
>>> --- a/src/common_init.c
>>> +++ b/src/common_init.c
>>> @@ -31,7 +31,9 @@
>>> +#include "config.h"
>>> #include "pciaccess.h"
>>> #include "pciaccess_private.h"
>>> @@ -122,6 +124,10 @@ pci_system_cleanup( void )
>>> +#ifdef HAVE_MTRR
>>> + if (pci_sys->mtrr_fd != -1)
>>> + close(pci_sys->mtrr_fd);
>>> free( pci_sys );
>>> pci_sys = NULL;
>>> xorg at lists.freedesktop.org: X.Org support
>>> Archives: http://lists.freedesktop.org/archives/xorg
>>> Info: http://lists.freedesktop.org/mailman/listinfo/xorg
>>> Your subscription address: jeremyhu at freedesktop.org
More information about the xorg