Re: [PATCH v2] uload unused input modules

2012-07-23 Thread Michal Suchanek 
Hello,

trying to send updated patches again.

There is not much actual code in common due to the use of the list
macros in this new series.

Thanks

Michal
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2] uload unused input modules

2012-07-17 Thread Michal Suchanek 
On 11 July 2012 08:46, Peter Hutterer peter.hutte...@who-t.net wrote:
 On Tue, Jul 03, 2012 at 12:45:21PM +0200, Michal Suchanek wrote:

 Signed-off-by: Michal Suchanek hramr...@gmail.com
 ---
  hw/xfree86/loader/loadmod.c |   19 ---
  1 file changed, 16 insertions(+), 3 deletions(-)

 diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c
 index 706b9b3..e94265a 100644
 --- a/hw/xfree86/loader/loadmod.c
 +++ b/hw/xfree86/loader/loadmod.c
 @@ -1103,10 +1103,23 @@ UnloadModuleOrDriver(ModuleDescPtr mod)
  LoaderUnload(mod-name, mod-handle);
  }

 -if (mod-child)
 +while (mod-child)
  UnloadModuleOrDriver(mod-child);
 -if (mod-sib)
 -UnloadModuleOrDriver(mod-sib);
 +
 +if (mod-parent) {
 +ModuleDescPtr sib_list = mod-parent-child;
 +if (sib_list == mod) {
 +mod-parent-child = mod-sib;
 +} else {
 +while (sib_list) {
 +if (sib_list-sib == mod) {
 +sib_list-sib = mod-sib;
 +break;
 +}
 +sib_list = sib_list-sib;
 +}
 +}

 we've got a bunch of macros to deal with null-terminated lists, so I've
 changed the second part to:

 if (mod-parent)
 nt_list_del(mod, mod-parent-child, ModuleDesc, sib)

 which does the same thing as above. read as
 delete mod from mod-parent-child, mod is of type ModuleDesc, sib is
 the pointer in mod to point to the next element.

 applied otherwise, thanks.


Too early.

There is a RemoveChild that does just this, with another copy of this code.

Going to send an updated patchset when I get to looking at the others, too.

Thanks

Michal
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2] uload unused input modules

2012-07-16 Thread Michal Suchanek 
On 11 July 2012 08:50, Peter Hutterer peter.hutte...@who-t.net wrote:
 On Mon, Jul 02, 2012 at 03:30:02PM +0200, Michal Suchanek wrote:

  urgh, no. why does the caller need to care about the module count at all? 
  the
  input code should just call unload and let the loader sort it out.

 urgh, yes.

 The input code needs to know when to delete the driver from its list
 and the loader does not return anything.

 I could possibly make it return something different when this check
 fails but I am not sure it has any nice semantics. The close on the
 actual object handle can fail also and then the module is already
 uninitialized, and unloading child submodules can fail, too.

 the bits your changing already change the ABI, so you've got (more or less)
 free range with the API as well. shape it into something that makes sense.
 would be a novel thing for this bit of code, anyways ;)

As I understand from the discussion of this patches and the other one
about API/ABI this is actually not a change to either.

The ModuleDesc structure is extended but it needs to be allocated
using a loader function. The functions that are the supposed outside
interface even cast pointer to these to void pointers.

The added CanUnloadModule is used in the interface between xf86Helper
and the loader, and there is no real API there, it's internal to the X
server and very interweawed.


   if (xf86InputDriverList[drvIndex]  
  xf86InputDriverList[drvIndex]-module)
   UnloadModule(xf86InputDriverList[drvIndex]-module);
   free(xf86InputDriverList[drvIndex]);
   xf86InputDriverList[drvIndex] = NULL;
  +if (drvIndex + 1  xf86NumInputDrivers)
  +memmove(xf86InputDriverList[drvIndex], 
  xf86InputDriverList[drvIndex+1],
  +sizeof(xf86InputDriverList[drvIndex]) * 
  (xf86NumInputDrivers - drvIndex - 1));
  +xf86NumInputDrivers--;
  +xf86InputDriverList[xf86NumInputDrivers] = NULL;
 
  this sounds like a prime target for a xorg_list switch to avoid this code.
 

 The loader sibling list even more so.

 Still the input device list is substantially simpler structure than
 xorg_list with the only downside that deletion is somewhat hairy.
 Perhaps another one is that xorg_list has tests but the input device
 list does not.

 that's my main argument. we've found quite a few bugs with various
 open-coded lists (or manual list resizing) that just using the macros is
 much safer. the nt_list_* macros for already existing, null-terminated lists
 and the xorg_list_* macros for anything new.


The xf86InputDriverList is defined in xf86Globals but it seems to be
internal to the xf86Helper.c functions so it can be safely changed to
something else I guess. It is a list of pointers, and the only
function that took index into the list rather than one of the pointers
stored in the list was unused.

It is mentioned in ddxDesign, though. Supposedly you can build a
static X server where these arrays are built at compile time. Is that
still true? I don't see the code for it.

On 11 July 2012 08:46, Peter Hutterer peter.hutte...@who-t.net wrote:
 On Tue, Jul 03, 2012 at 12:45:21PM +0200, Michal Suchanek wrote:
 Hello,

 since the patches got lost somewhere without even generating an error
 sending as plain email.

 Changes since v1:

  - split out module 'query' functions
  - rename some variables
  - fix some whitespace and braces

 Thanks

 the patches got mangled again :(
 I could copy/paste the first one into git am directly, number 3 is
 definitely busted though.

I can send a link to a git branch along with the patches in the future
but sending bulk emails obviously does not work, likely due to spam
prevention measures.


 looks a lot better, thanks. I even think actually understand most of it,
 though the loader is a bit obtuse :)
 I'm still not a big fan of the API, especially the one exposed to bits
 outside the loader. I think there's still a bit of room for
 improvement. What we do need for this patch series is a set of simple tests
 that make sure module loading and unloading works as before and doesn't
 mangle any pointers or other bits. I _think_ this should be possible to add
 by declaring a couple of structs and passing them through the various
 LoadModule/UnloadModule. should help us to find potential memory leaks too.

The thing is the patch only formalizes an internal api between
xf86Helper and the loader. The input driver list only contains the
original modules (in the past never unloaded) and devices only get
duplicates (unloaded on device teardown, at least recently).

Some tests with a dummy loader might be nice but I am not proficient
with gtest or what the x server uses and writing a dummy loader would
take some time.

It should probably include tests with module version info, the version
check code is very weird.



 if you're changing the module ABI anyway, we can change the InputDriverRec
 too and add a struct list so we don't need to do the memmove/drvIndex dance
 in the

Re: [PATCH v2] uload unused input modules

2012-07-11 Thread Peter Hutterer 
On Tue, Jul 03, 2012 at 12:45:21PM +0200, Michal Suchanek wrote:
 Hello,
 
 since the patches got lost somewhere without even generating an error
 sending as plain email.
 
 Changes since v1:
 
  - split out module 'query' functions
  - rename some variables
  - fix some whitespace and braces
 
 Thanks

the patches got mangled again :(
I could copy/paste the first one into git am directly, number 3 is
definitely busted though.

looks a lot better, thanks. I even think actually understand most of it,
though the loader is a bit obtuse :)
I'm still not a big fan of the API, especially the one exposed to bits
outside the loader. I think there's still a bit of room for
improvement. What we do need for this patch series is a set of simple tests
that make sure module loading and unloading works as before and doesn't
mangle any pointers or other bits. I _think_ this should be possible to add
by declaring a couple of structs and passing them through the various
LoadModule/UnloadModule. should help us to find potential memory leaks too.
 
 From a86d41ee113f76776a9410205ef6f90c78e41d22 Mon Sep 17 00:00:00 2001
 From: Michal Suchanek hramr...@gmail.com
 Date: Mon, 4 Jun 2012 14:22:14 +0200
 Subject: [PATCH 1/4] xfree86/loader: Do not unload sibling modules.
 To: xorg-devel@lists.x.org
 
 Ordering of sibling modules is internal to the loader implementation.
 
 Unloading siblings following the requested module in the sibling list
 leads to unexpected unloading of seemingly random modules.
 
 Signed-off-by: Michal Suchanek hramr...@gmail.com
 ---
  hw/xfree86/loader/loadmod.c |   19 ---
  1 file changed, 16 insertions(+), 3 deletions(-)
 
 diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c
 index 706b9b3..e94265a 100644
 --- a/hw/xfree86/loader/loadmod.c
 +++ b/hw/xfree86/loader/loadmod.c
 @@ -1103,10 +1103,23 @@ UnloadModuleOrDriver(ModuleDescPtr mod)
  LoaderUnload(mod-name, mod-handle);
  }
 
 -if (mod-child)
 +while (mod-child)
  UnloadModuleOrDriver(mod-child);
 -if (mod-sib)
 -UnloadModuleOrDriver(mod-sib);
 +
 +if (mod-parent) {
 +ModuleDescPtr sib_list = mod-parent-child;
 +if (sib_list == mod) {
 +mod-parent-child = mod-sib;
 +} else {
 +while (sib_list) {
 +if (sib_list-sib == mod) {
 +sib_list-sib = mod-sib;
 +break;
 +}
 +sib_list = sib_list-sib;
 +}
 +}

we've got a bunch of macros to deal with null-terminated lists, so I've
changed the second part to:

if (mod-parent)
nt_list_del(mod, mod-parent-child, ModuleDesc, sib) 

which does the same thing as above. read as
delete mod from mod-parent-child, mod is of type ModuleDesc, sib is
the pointer in mod to point to the next element.

applied otherwise, thanks.

 +}
  free(mod-path);
  free(mod-name);
  free(mod);
 -- 
 1.7.10
 
 From 04aee58b6b497613a9b0c2c9253cfbcbc3f7e249 Mon Sep 17 00:00:00 2001
 From: Michal Suchanek hramr...@gmail.com
 Date: Mon, 2 Jul 2012 10:54:53 +0200
 Subject: [PATCH 2/4] xfree86/loader: Split off some loader code into separate
  query functions.
 To: xorg-devel@lists.x.org
 
  * Add CanUnloadModule
 
 Signed-off-by: Michal Suchanek hramr...@gmail.com
 ---
  hw/xfree86/loader/loaderProcs.h |5 +
  hw/xfree86/loader/loadmod.c |   36 ++--
  2 files changed, 39 insertions(+), 2 deletions(-)
 
 diff --git a/hw/xfree86/loader/loaderProcs.h b/hw/xfree86/loader/loaderProcs.h
 index a7b752b..c33039a 100644
 --- a/hw/xfree86/loader/loaderProcs.h
 +++ b/hw/xfree86/loader/loaderProcs.h
 @@ -88,6 +88,11 @@ unsigned long LoaderGetModuleVersion(ModuleDescPtr mod);
  void LoaderResetOptions(void);
  void LoaderSetOptions(unsigned long);
 
 +int IsBuiltinModule(ModuleDescPtr mod);
 +int IsDuplicated(ModuleDescPtr mod);
 +int IsDuplicateModule(ModuleDescPtr mod);
 +int CanUnloadModule(ModuleDescPtr mod);
 +
  /* Options for LoaderSetOptions */
  #define LDR_OPT_ABI_MISMATCH_NONFATAL0x0001
 
 diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c
 index e94265a..4844e18 100644
 --- a/hw/xfree86/loader/loadmod.c
 +++ b/hw/xfree86/loader/loadmod.c
 @@ -803,6 +803,19 @@ NewModuleDesc(const char *name)
  return mdp;
  }
 
 +/* An advisory function. When true is returned unloading the module should be
 + * safe. Uload can be attempted nonetheless. */
 +int CanUnloadModule(ModuleDescPtr mod)
 +{
 +if (IsBuiltinModule(mod))
 +return 0;
 +if (IsDuplicated(mod))
 +return 0;
 +/* Neither of the above can be true for modules for which 
 IsDuplicateModule
 + * is true. */
 +return 1;
 +}
 +
  ModuleDescPtr
  DuplicateModule(ModuleDescPtr mod, ModuleDescPtr parent)
  {
 @@ -1083,10 +1096,29 @@ UnloadModule(pointer mod)
  UnloadModuleOrDriver((ModuleDescPtr) mod);
  }
 
 +/* Is