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. > > Thanks, > Nithin. > > >> >> 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" >>> error. >>> >>> Signed-off-by: Nithin Nayak Sujir<[email protected]> >>> --- >>> 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<stdlib.h> >>> #include<errno.h> >>> +#include<unistd.h> >>> >>> +#include "config.h" >>> #include "pciaccess.h" >>> #include "pciaccess_private.h" >>> >>> @@ -122,6 +124,10 @@ pci_system_cleanup( void ) >>> (*pci_sys->methods->destroy)(); >>> } >>> >>> +#ifdef HAVE_MTRR >>> + if (pci_sys->mtrr_fd != -1) >>> + close(pci_sys->mtrr_fd); >>> +#endif >>> free( pci_sys ); >>> pci_sys = NULL; >>> } >>> -- >>> 1.7.1 >>> >>> >>> _______________________________________________ >>> [email protected]: X.Org support >>> Archives: http://lists.freedesktop.org/archives/xorg >>> Info: http://lists.freedesktop.org/mailman/listinfo/xorg >>> Your subscription address: [email protected] >>> >> >> > _______________________________________________ [email protected]: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: [email protected]
