[PATCH] libpciaccess: close mtrr fd on pci_cleanup

[Jeremy Huddleston]

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<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<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
>>> 
>>> 
>>> _______________________________________________
>>> 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
>>> 
>> 
>> 
>