[PATCH] libpciaccess: close mtrr fd on pci_cleanup

[Nithin Sujir]

On 10/24/2011 10:49 AM, Jeremy Huddleston wrote:
> 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.

Sounds good. I'll send another patch shortly.

Thanks,
Nithin.


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