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

Re: [libvirt] Availability of libvirt-4.6.0 Release Candidate 2



On Wed, Aug 01, 2018 at 01:10:08PM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 01, 2018 at 01:41:48PM +0200, Bjoern Walk wrote:
> > Daniel P. Berrangé <berrange redhat com> [2018-08-01, 11:51AM +0100]:
> > > On Tue, Jul 31, 2018 at 03:54:43PM +0200, Bjoern Walk wrote:
> > > > Bjoern Walk <bwalk linux ibm com> [2018-07-31, 03:16PM +0200]:
> > > > > I have not yet had the time to figure out what goes wrong, any ideas are welcome.
> > > > 
> > > > Ah, classic. The mocked virRandomBytes function is not endian-agnostic,
> > > > generating a different interface name as expected by the test.
> > > 
> > > I'm not seeing why virRandomBytes is affected by endian-ness. It is simply
> > > populating an array of bytes, so there's no endin issues to consider there.
> > 
> > Ahem, we are writing linearly to a byte array, of course this is
> > dependend on the endianness of the architecture. The actual problem is
> > that the mocked version does _not_ provide a random value, but a
> > deterministic one, which is byte order reversed on big endianness.
> 
> There's no concept of reversed byte order when you're accessing an
> array of bytes.  If you write elements 0, 1, 2, ...7, etc and then
> the caller reads elements 0, 1,2, ..., 7 everything is fine.
> 
> Endianness only comes into play if you take that array of bytes
> and then cast/assign it to a larger type were endianess is
> relevant (eg int16/int32/int64)
> 
> eg
> 
>    char bytes[8];
> 
>    virRandomBytes(bytes, 8);
> 
>    uint64_t val = (uint64_t)bytes;
> 
> the problem in such a case isn't virRandomBytes, it is the cast
> to the larger sized integer type.
> 
> > > Can you elaborate on the actual error messages you are getting from the
> > > tests, and what aspect makes you think virRandomBytes is the problem ?
> > 
> > The name of the interface is wrong compared to what is explicitly
> > expected in the test case:
> > 
> > 200         if (virAsprintf(&temp_ifacename,
> > (gdb)
> > 205         VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'",
> > (gdb) p temp_ifacename
> > $1 = 0x1014320 "libvirt-iface-04050607"
> > 
> > > Seems more likely that it is something higher up the call stack.
> 
> That looks like it uses virRandomBits rather than virRandomBytes.

Yes, it is virRandomBits that is the problem - it is taking a uint64_t
variable and casting it to a "unsigned char *", which is not an
endian safe thing todo.

We need to rewrite virRandomBits to do

   char val[8];

   virRandomBytes(val, sizeof(val));

   uint64_t ret =
      val[0] |
      ((uint64_t)val[1]) << 8 |
      ((uint64_t)val[2]) << 16 |
      ((uint64_t)val[3]) << 24 |
      ((uint64_t)val[4]) << 32 |
      ((uint64_t)val[5]) << 40 |
      ((uint64_t)val[6]) << 48 |
      ((uint64_t)val[7]) << 56 |

   return ret & ((1ULL << nbits) -1);

to make it endiansafe.

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 :|


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