[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] [PATCH] libvirt.c: warning: dereferencing type-punned pointer will break strict-aliasing rules



On Fri, Mar 02, 2007 at 11:45:04AM +0000, Mark McLoughlin wrote:
> On Fri, 2007-03-02 at 11:30 +0000, Richard W.M. Jones wrote:
> > I'm currently trying to get libvirt to compile with -Werror.  One 
> > problem which came up early is the warning in $SUBJECT.  The gcc info 
> > page (see -fstrict-aliasing) is pretty unclear about what exactly causes 
> > this problem, so the attached patch rewrites the code quite 
> > conservatively to avoid the problem.
> 
> 	Uggh, -fstrict-aliasing is the bane of all our lives. Whoever thought
> it was a good idea? Wonder how much this optimisation actually gives us?
> Is it enough to justify all this? Grr.
> 
> 	(Deep breath)
> 
> 	Does something like this work:
> 
> struct _virDriver {
>     const char *name;
> };
> 
> struct _virDomainDriver {
>     struct _virDriver base;
>     int no;
>     unsigned long ver;
>     virDrvOpen open;
> };
> 
> struct _virNetworkDriver {
>     struct _virDriver base;
>     virDrvOpen open;
> };

This feels kind of sick - inventing a common shared struct between the
two driver tables, when there isn't any common stuff to share :-(

Looking at the code, IMHO, the whole approach of iterating over the driver
table soo many times is just wrong, when we can simply have an integer
count recording how many drivers are registered. This eliminates both
for(;;) loops, and reduces the amount of code to the point where I don't
think there's anything to be gained by having a generic _virDriverRegister
with all the type-casting this entails.

So how about the attached patch instead....

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
Index: libvirt.c
===================================================================
RCS file: /data/cvs/libvirt/src/libvirt.c,v
retrieving revision 1.59
diff -u -p -r1.59 libvirt.c
--- libvirt.c	6 Mar 2007 21:55:44 -0000	1.59
+++ libvirt.c	6 Mar 2007 22:49:06 -0000
@@ -42,7 +42,9 @@
  */
 
 static virDriverPtr virDriverTab[MAX_DRIVERS];
+static int virDriverTabCount = 0;
 static virNetworkDriverPtr virNetworkDriverTab[MAX_DRIVERS];
+static int virNetworkDriverTabCount = 0;
 static int initialized = 0;
 
 /**
@@ -57,8 +59,6 @@ static int initialized = 0;
 int
 virInitialize(void)
 {
-    int i;
-
     if (initialized)
         return(0);
     initialized = 1;
@@ -67,14 +67,6 @@ virInitialize(void)
         return (-1);
 
     /*
-     * should not be needed but...
-     */
-    for (i = 0;i < MAX_DRIVERS;i++) {
-         virDriverTab[i] = NULL;
-         virNetworkDriverTab[i] = NULL;
-    }
-
-    /*
      * Note that the order is important the first ones have a higher priority
      */
     xenHypervisorRegister();
@@ -163,35 +155,6 @@ virLibNetworkError(virNetworkPtr network
                     errmsg, info, NULL, 0, 0, errmsg, info);
 }
 
-static int
-_virRegisterDriver(void *driver, int isNetwork)
-{
-    void **drivers;
-    int i;
-
-    if (!initialized)
-        if (virInitialize() < 0)
-	    return -1;
-
-    if (driver == NULL) {
-        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
-	return(-1);
-    }
-    drivers = isNetwork ? (void **) virNetworkDriverTab : (void **) virDriverTab;
-    for (i = 0;i < MAX_DRIVERS;i++) {
-        if (drivers[i] == driver)
-	    return(i);
-    }
-    for (i = 0;i < MAX_DRIVERS;i++) {
-        if (drivers[i] == NULL) {
-	    drivers[i] = driver;
-	    return(i);
-	}
-    }
-    virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
-    return(-1);
-}
-
 /**
  * virRegisterNetworkDriver:
  * @driver: pointer to a network driver block
@@ -203,7 +166,21 @@ _virRegisterDriver(void *driver, int isN
 int
 virRegisterNetworkDriver(virNetworkDriverPtr driver)
 {
-    return _virRegisterDriver(driver, 1);
+    if (virInitialize() < 0)
+      return -1;
+
+    if (driver == NULL) {
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
+	return(-1);
+    }
+
+    if (virNetworkDriverTabCount >= (sizeof(virNetworkDriverTab)/sizeof(virNetworkDriverPtr))) {
+    	virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
+	return(-1);
+    }
+
+    virNetworkDriverTab[virNetworkDriverTabCount] = driver;
+    return virNetworkDriverTabCount++;
 }
 
 /**
@@ -217,7 +194,21 @@ virRegisterNetworkDriver(virNetworkDrive
 int
 virRegisterDriver(virDriverPtr driver)
 {
-    return _virRegisterDriver(driver, 0);
+    if (virInitialize() < 0)
+      return -1;
+
+    if (driver == NULL) {
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
+	return(-1);
+    }
+
+    if (virDriverTabCount >= (sizeof(virDriverTab)/sizeof(virDriverPtr))) {
+    	virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
+	return(-1);
+    }
+
+    virDriverTab[virDriverTabCount] = driver;
+    return virDriverTabCount++;
 }
 
 /**
@@ -252,14 +243,14 @@ virGetVersion(unsigned long *libVer, con
     if (typeVer != NULL) {
         if (type == NULL)
 	    type = "Xen";
-	for (i = 0;i < MAX_DRIVERS;i++) {
+	for (i = 0;i < virDriverTabCount;i++) {
 	    if ((virDriverTab[i] != NULL) &&
 	        (!strcmp(virDriverTab[i]->name, type))) {
 		*typeVer = virDriverTab[i]->ver;
 		break;
 	    }
 	}
-        if (i >= MAX_DRIVERS) {
+        if (i >= virDriverTabCount) {
             *typeVer = 0;
             virLibConnError(NULL, VIR_ERR_NO_SUPPORT, type);
             return (-1);
@@ -300,8 +291,8 @@ virConnectOpen(const char *name)
         goto failed;
     }
 
-    for (i = 0;i < MAX_DRIVERS;i++) {
-        if ((virDriverTab[i] != NULL) && (virDriverTab[i]->open != NULL)) {
+    for (i = 0;i < virDriverTabCount;i++) {
+        if ((virDriverTab[i]->open != NULL)) {
 	    res = virDriverTab[i]->open(ret, name, VIR_DRV_OPEN_QUIET);
 	    /*
 	     * For a default connect to Xen make sure we manage to contact
@@ -316,8 +307,8 @@ virConnectOpen(const char *name)
 	}
     }
 
-    for (i = 0;i < MAX_DRIVERS;i++) {
-        if ((virNetworkDriverTab[i] != NULL) && (virNetworkDriverTab[i]->open != NULL) &&
+    for (i = 0;i < virNetworkDriverTabCount;i++) {
+        if ((virNetworkDriverTab[i]->open != NULL) &&
 	    (res = virNetworkDriverTab[i]->open(ret, name, VIR_DRV_OPEN_QUIET)) == 0) {
             ret->networkDrivers[ret->nb_network_drivers++] = virNetworkDriverTab[i];
         }
@@ -375,15 +366,17 @@ virConnectOpenReadOnly(const char *name)
         goto failed;
     }
 
-    for (i = 0;i < MAX_DRIVERS;i++) {
-        if ((virDriverTab[i] != NULL) && (virDriverTab[i]->open != NULL)) {
+    for (i = 0;i < virDriverTabCount;i++) {
+        if ((virDriverTab[i]->open != NULL)) {
 	    res = virDriverTab[i]->open(ret, name,
 	                                VIR_DRV_OPEN_QUIET | VIR_DRV_OPEN_RO);
 	    if (res == 0)
 	        ret->drivers[ret->nb_drivers++] = virDriverTab[i];
 
 	}
-        if ((virNetworkDriverTab[i] != NULL) && (virNetworkDriverTab[i]->open != NULL) &&
+    }
+    for (i = 0;i < virNetworkDriverTabCount;i++) {
+        if ((virNetworkDriverTab[i]->open != NULL) &&
 	    (res = virNetworkDriverTab[i]->open(ret, name, VIR_DRV_OPEN_QUIET)) == 0) {
             ret->networkDrivers[ret->nb_network_drivers++] = virNetworkDriverTab[i];
         }

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]