[ANNOUNCE] xorg-server 1.7.99.902

[Dan Nicholson]

On Wed, Mar 24, 2010 at 06:05:27AM +0100, Stefan Dirsch wrote:
> On Mon, Mar 22, 2010 at 07:59:18AM -0700, Dan Nicholson wrote:
> > 2. Make the handling of missing sections from an existing
> > configuration behave more like the full autoconfig. In other words, if
> > there's a missing Screen section, generate multiple Screen/Device
> > pairs with fallbacks. I think this is the correct fix, but I'm not
> > that familiar with the autoconfig code so I don't think I could whip
> > up a patch that fast.
> 
> I've now attached a patch to
> 
>   https://bugs.freedesktop.org/show_bug.cgi?id=27229
> 
> which does that. It works fine for us.

Looks pretty good from my understanding, but I'd like to polish it a bit more
before applying it. I'm gonna do the review here since I'm not that familiar
with that part of the code.

The first thing is that it would be great if this was a git formatted patch
with a commit message. But if it's easier to keep it as a separate patch, can
you add some information to the patch header? It would also be great if there
could be some comments in the code. It looks like multiple Screen sections are
being handled, but not really.

Now to the patch.

--- hw/xfree86/common/xf86AutoConfig.c
+++ hw/xfree86/common/xf86AutoConfig.c
@@ -539,34 +541,13 @@
     }
 }
 
-static char*
-chooseVideoDriver(void)
-{
-    char *chosen_driver = NULL;
-    int i;
-    char *matches[20]; /* If we have more than 20 drivers we're in trouble */
-
-    listPossibleVideoDrivers(matches, 20);
-
-    /* TODO Handle multiple drivers claiming to support the same PCI ID */
-    chosen_driver = matches[0];
-
-    xf86Msg(X_DEFAULT, "Matched %s for the autoconfigured driver\n",
-	    chosen_driver);
-
-    for (i = 0; matches[i] ; i++) {
-        if (matches[i] != chosen_driver) {
-            xfree(matches[i]);
-        }
-    }
-
-    return chosen_driver;
-}
-
 GDevPtr
 autoConfigDevice(GDevPtr preconf_device)
 {
-    GDevPtr ptr = NULL;
+    GDevPtr ptr = NULL, cptr = NULL;
+    char *matches[20]; /* If we have more than 20 drivers we're in trouble */
+    int num_matches = 0, num_screens = 0, i;
+    screenLayoutPtr slp;
 
     if (!xf86configptr) {
         return NULL;
@@ -589,14 +571,49 @@
         ptr->driver = NULL;
     }
     if (!ptr->driver) {
-        ptr->driver = chooseVideoDriver();
+	listPossibleVideoDrivers(matches, 20);
+	for (; matches[num_matches] ; num_matches++);

Coding style issues for here and the rest of the patch.

* Let's not mix tabs and spaces. The rest of this function uses all spaces.
* We don't typically pad the parameters with trailing spaces.
* Wrap at 78 columns unless it becomes really unreadable.
* Spaces between operators, please. (i=1;i<num_screens;i++) is not very
  readable.
* Please add a blank line here or there to break up the action.

+	slp = xf86ConfigLayout.screens;
+	if (slp) {
+	    for (; slp[num_screens].screen ; num_screens++);
+	    xf86ConfigLayout.screens = xnfcalloc(1,(num_screens+num_matches+1) * sizeof(screenLayoutRec));
+	    xf86ConfigLayout.screens[0] = slp[0];
+	}
+	for (i=0; i<num_matches;i++) {
+	    if (i==0) {
+		ptr->driver = matches[0];
+		if (slp && !xf86ConfigLayout.screens[0].screen->device) {
+		    xf86ConfigLayout.screens[0].screen->device = ptr;
+		    ptr->myScreenSection = xf86ConfigLayout.screens[0].screen;
+		}
+	    } else {
+		if (slp) {
+		    xf86ConfigLayout.screens[i].screen = xnfcalloc(1, sizeof(confScreenRec));
+		    if(!xf86ConfigLayout.screens[i].screen)
+			return NULL;
+		    memcpy(xf86ConfigLayout.screens[i].screen, slp[0].screen, sizeof(confScreenRec));
+		}
+		cptr = xcalloc(1, sizeof(GDevRec));

It seems were making the decision here that if you had one Screen section,
you'll want more of them. Otherwise, we're just allocating another GDevPtr and
doing nothing with it. Probably the right thing to do is generate Screen
sections anyway, but that might be a more involved change. Here we should
probably return the first autoconfigured device (ptr) if there are no screen
sections and skip this whole loop.

+		if (!cptr)
+		    return NULL;
+		memcpy(cptr, ptr, sizeof(GDevRec));
+		cptr->identifier = xnfcalloc(1,strlen("Autoconfigured Video Device ")+strlen(matches[i])+1);
+		sprintf(cptr->identifier, "Autoconfigured Video Device %s", matches[i]);

Seems you could use xnfalloc since you immediately memcpy/sprintf over the
allocations anyway.

+		cptr->driver = matches[i];
+		if (slp) {
+		    xf86ConfigLayout.screens[i].screen->device = cptr;
+		    cptr->myScreenSection = xf86ConfigLayout.screens[i].screen;
+		}
+	    }
+	}
+	for (i=1;i<num_screens;i++) {
+		xf86ConfigLayout.screens[i+num_matches] = slp[i];
+	}

Oh, I see. You're putting the rest of the existing Screen sections at the end
of the array. That's nice.

+	xf86ConfigLayout.screens[num_screens+num_matches].screen = NULL;
+	xfree(slp);
     }
 
-    /* TODO Handle multiple screen sections */
-    if (xf86ConfigLayout.screens && !xf86ConfigLayout.screens->screen->device) {
-        xf86ConfigLayout.screens->screen->device = ptr;
-        ptr->myScreenSection = xf86ConfigLayout.screens->screen;
-    }
+    /* TODO Handle rest of multiple screen sections */

This TODO comment should be moved up above that last loop where you're just
shifting the rest of the screens to the end of the array.

     xf86Msg(X_DEFAULT, "Assigned the driver to the xf86ConfigLayout\n");
 
     return ptr;

--
Dan