[Libvir] [PATCH] libvirt.c: warning: dereferencing type-punned pointer will break strict-aliasing rules
5:30 a.m.
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.
Rich.
--
Emerging Technologies, Red Hat http://et.redhat.com/~rjones/
64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421
"[Negative numbers] darken the very whole doctrines of the equations
and make dark of the things which are in their nature excessively
obvious and simple" (Francis Maseres FRS, mathematician, 1759)
5:45 a.m.
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;
};
static int
_virRegisterDriver(virDriver *driver, int isNetwork)
{
virDriver *drivers;
drivers = (virDriver *) isNetwork ? virNetworkDriverTab : virDriverTab;
...
}
I started re-factoring qemud like this so as to have a base object from
which domains and networks derive so that we don't have to have as much
copied and pasted code between the two, but it's a pretty big task.
Cheers,
Mark.
4:52 p.m.
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 -=|
1:25 a.m.
On Tue, 2007-03-06 at 22:52 +0000, Daniel P. Berrange wrote:
This feels kind of sick - inventing a common shared struct between
the
two driver tables, when there isn't any common stuff to share :-(
Yeah. Although, as a way to share more code between domains and
networks (especially in qemud), it might make sense.
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.
That sounds fine.
@@ -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))) {
Just use MAX_DRIVERS?
Cheers,
Mark.
8:50 a.m.
On Wed, Mar 07, 2007 at 07:25:59AM +0000, Mark McLoughlin wrote:
On Tue, 2007-03-06 at 22:52 +0000, Daniel P. Berrange wrote:
> This feels kind of sick - inventing a common shared struct between the
> two driver tables, when there isn't any common stuff to share :-(
Yeah. Although, as a way to share more code between domains and
networks (especially in qemud), it might make sense.
> 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.
That sounds fine.
> @@ -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))) {
Just use MAX_DRIVERS?
Committed with this change applied.
I also fixed virConnectOpenReadonly which was not passing the readonly
flag down to the network driver.
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 -=|
3:52 a.m.
Daniel P. Berrange wrote:
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.
A kind of "Shlemiel the painter"[1] problem?
So how about the attached patch instead....
I think that looks better.
Rich.
[1] http://www.joelonsoftware.com/articles/fog0000000319.html
--
Emerging Technologies, Red Hat http://et.redhat.com/~rjones/
64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421
"[Negative numbers] darken the very whole doctrines of the equations
and make dark of the things which are in their nature excessively
obvious and simple" (Francis Maseres FRS, mathematician, 1759)
6469
days inactive
6475
days old
5 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Mark McLoughlin -
Richard W.M. Jones