[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freedesktop-xorg
Subject:    Re: [PATCH] libpciaccess: close mtrr fd on pci_cleanup
From:       "Nithin Sujir" <nsujir () broadcom ! com>
Date:       2011-10-24 18:01:39
Message-ID: 4EA5A803.1090505 () broadcom ! com
[Download RAW message or body]


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@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@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@freedesktop.org
> > > > 
> > > 
> > > 
> > 
> 
> 

_______________________________________________
xorg@lists.freedesktop.org: X.Org support
Archives: http://lists.freedesktop.org/archives/xorg
Info: http://lists.freedesktop.org/mailman/listinfo/xorg
Your subscription address: freedesktop-xorg@progressive-comp.com


[prev in list] [next in list] [prev in thread] [next in thread]