[libvirt] [PATCH 2/2] Revert "Prevent more compiler optimization of mockable functions"

[Daniel P. Berrange]

On Wed, Jul 12, 2017 at 01:56:47PM +0200, Martin Kletzander wrote:
> On Wed, Jul 12, 2017 at 01:49:04PM +0200, Martin Kletzander wrote:
> > On Wed, Jul 12, 2017 at 01:10:08PM +0200, Martin Kletzander wrote:
> > > On Wed, Jul 12, 2017 at 11:14:16AM +0100, Daniel P. Berrange wrote:
> > > > This reverts commit e4b980c853d2114b25fa805a84ea288384416221.
> > > > 
> > > > When a binary links against a .a archive (as opposed to a shared library),
> > > > any symbols which are marked as 'weak' get silently dropped. As a result
> > > > when the binary later runs, those 'weak' functions have an address of
> > > > 0x0 and thus crash when run.
> > > > 
> > > > This happened with virtlogd and virtlockd because they don't link to
> > > > libvirt.so, but instead just libvirt_util.a and libvirt_rpc.a. The
> > > > virRandomBits symbols was weak and so left out of the virtlogd &
> > > > virtlockd binaries, despite being required by virHashTable functions.
> > > > 
> > > > Various other binaries like libvirt_lxc, libvirt_iohelper, etc also
> > > > link directly to .a files instead of libvirt.so, so are potentially
> > > > at risk of dropping symbols leading to a later runtime crash.
> > > > 
> > > > This is normal linker behaviour because a weak symbol is not treated
> > > > as undefined, so nothing forces it to be pulled in from the .a You
> > > > have to force the linker to pull in weak symbols using -u$SYMNAME
> > > > which is not a practical approach.
> > > > 
> > > 
> > > How is this done by gnulib (or libc) when most their functions are weak
> > > aliases anyway?  Can't we use the same approach they have?
> > > virtlo{g,ck}d link with libgnu.la as well and there is no problem with
> > > that, right?  So I guess this _must_ be solvable somehow, IMHO.
> > > 
> > > I'm just curious how that works.
> > > 
> > > Martin
> > 
> > I guess we would have to do something like the following, but for every
> > function.
> > 
> > diff --git i/src/util/virrandom.c w/src/util/virrandom.c
> > index 41daa404b273..3d9fe7f85d97 100644
> > --- i/src/util/virrandom.c
> > +++ w/src/util/virrandom.c
> > @@ -99,7 +99,7 @@ VIR_ONCE_GLOBAL_INIT(virRandom)
> >  *
> >  * Return: a random number with @nbits entropy
> >  */
> > -uint64_t virRandomBits(int nbits)
> > +static uint64_t __virRandomBits(int nbits)
> > {
> >     uint64_t ret = 0;
> >     int32_t bits;
> > @@ -125,6 +125,7 @@ uint64_t virRandomBits(int nbits)
> >     virMutexUnlock(&randomLock);
> >     return ret;
> > }
> > +uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE __attribute__((alias("__virRandomBits")));
> > 
> > 
> > /**
> > diff --git i/src/util/virrandom.h w/src/util/virrandom.h
> > index 990a456addf7..abda95aef506 100644
> > --- i/src/util/virrandom.h
> > +++ w/src/util/virrandom.h
> > @@ -24,7 +24,7 @@
> > 
> > # include "internal.h"
> > 
> > -uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE;
> > +uint64_t virRandomBits(int nbits);
> > double virRandom(void);
> > uint32_t virRandomInt(uint32_t max);
> > int virRandomBytes(unsigned char *buf, size_t buflen)
> > --
> > 
> > And of course that could be macrofied so that ATTRIBUTE_MOCKABLE takes
> > function or something, etc.
> > 
> > I like this more than reverting the patches.
> > 
> 
> Also, having the weak alias we can drop all the mocks and the problems
> with them and just redefine the functions we would like to mock in the
> tests (see tests/virfilewrapper.c), it would work on win32, it would
> not compile if we would forgot to make the function as an alias (so no
> need to check that in the syntax-check) and maybe provide other benefits
> that I can't think of right now.

Empirically I'm not seeing any compiler warnings if i take this approach
and don't mark the function as an alias. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|