[tiago.vignatti at nokia.com: [RFC] Make screen recs run-time adjustable]

[Jamey Sharp]

[
 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, <tiago.vignatti at nokia.com> 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 <jamey at minilop.net>

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