[ We've been discussing off-list how best to make MAXSCREENS die. Now I think the rationales and discussion need to be public, archived, and reviewed by people who understand the server's history. Hope you don't mind, Tiago. The code in question is here: http://cgit.freedesktop.org/~vignatti/xserver/log/?h=maxscreenless except Jon Turney's patches currently have a newer version here: http://cgit.freedesktop.org/~jturney/xserver/log/?h=maxscreenless ]
On Wed, Apr 21, 2010 at 7:49 AM, <[email protected]> wrote: > On Wed, Apr 21, 2010 at 12:27:21AM +0200, ext Jamey Sharp wrote: >> Can you either explain to me why you want the SCREENSALLOC macro ... > > I just clarified better in the commit messages. Please, take a look on > the same repo&branch. Ah, I see. Yes, that helped. OK, for the record, I still don't like wrapping allocation up in these macros, but I'm losing my will to argue about it. :-) They do at least have the advantage that when we figure out what we should do instead, they're easy to find and rip out. :-) For 5afd4d244cec754ac6b340b8fb11378931466e8e ("dix: allocate global screen related arrays as needed"): I like it all except that deleting unused_screenPrivate seems questionable. A year ago ajax renamed that field, and wrote "The unused_ members are ABI spacing. Please reuse them." So it isn't a MAXSCREENS problem to leave it there (though it was when the original patch was written) and I guess deleting it is wrong? With that fixed, Reviewed-by: Jamey Sharp <[email protected]> In the "dix and others" patch, the micmap changes seem right, but I'm not convinced about fbcmap. The old patch would only allocate memory if the argument to SCREENSALLOC was NULL. I'm glad you removed that from the macros :-) but it breaks the fbcmap.c changes. Anyway, I've just sent out a patch that eliminates MAXSCREENS from fbcmap using screen privates; perhaps you'd like to pick that up instead? I guess explains the many assignments to NULL before calling SCREENSALLOC, as well. In the current version of the macro, all those NULL assignments are immediately killed by assigning from xalloc, and should be deleted. This affects the "dix and others" and xfree86 DDX patches. I *think* it's safe to say that all assignments or initializations to NULL that are introduced by these patches should be removed. For globals, they're redundant, and apparently inconsistent with the surrounding code style. For local variables, I'd like to get the compiler's warning about variables used without being initialized. I don't know which structs are part of ABI. In "dix and others", should the inputstr.h edit be called out as an ABI change? It doesn't look to me like the xfree86 patch could possibly be breaking ABI any more. Maybe that bit of the commit message needed to get moved to the newly-split earlier commits. Do you have time to split the xf86InitOrigins change into a separate commit? So screensLeft would change from long to int[MAXSCREENS] first, then that would be replaced by allocating xf86NumScreens entries. I think it will be easier for other people to review that way. The use of SCREENSFREE in the Xnest patch is clearly wrong: it's immediately followed by dereferencing the just-freed array. I haven't looked to see what it should do instead though. I'm guessing the xnestInstallColormap hunk is also wrong, but again I think my patch supercedes that. And I don't see why Args.c should need globals.h when it didn't before? I've been doing most of my patch review through cgit, which might be misleading me, but it looks like there is inconsistent indentation introduced by some of these patch hunks. You might want to check that. I'd especially worry if it's a sign that the old patches didn't apply to the code they were supposed to. So I think there's some stuff to clean up still, but pieces (including your ReallocGlobal bits and Jon's xwin patches) are basically ready now. As you mentioned off-list, somebody still needs to write the corresponding Xvfb changes (my colormap patch hits part of it), but then this cleanup work is done. I think most of the SCREENSALLOC uses would go away if there was an API for managing PanoramiXRes resources, which would be a much more sensible place to hook for hotpluggable screens. I'm toying with that idea; we'll see if it goes anywhere before your patch series lands. :-) >> functions like PanoramiXCreateWindow [shouldn't] kill the X >> server on failure to allocate memory ... > > that's true and I missed that. We should gracefully fail accordingly > the kind of protocol error. It should be okay now in my repository. That looks better, yes. :-) Thanks for putting all this work into killing MAXSCREENS, Tiago! Jamey _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
