[libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe

Bjoern Walk posted 1 patch 5 years, 7 months ago
Failed in applying to current master (apply log)
src/util/virrandom.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
[libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe
Posted by Bjoern Walk 5 years, 7 months ago
Make the generation of random bits in virRandomBits independent of the
endianness of the running architecture.

This also solves problems with the mocked random byte generation on
big-endian machines.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com>
---
This goes on top of Michal's fix: https://www.redhat.com/archives/libvir-list/2018-August/msg00080.html

 src/util/virrandom.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/util/virrandom.c b/src/util/virrandom.c
index 7915f653..26ff68f5 100644
--- a/src/util/virrandom.c
+++ b/src/util/virrandom.c
@@ -34,6 +34,7 @@
 # include <gnutls/crypto.h>
 #endif
 
+#include "virendian.h"
 #include "virrandom.h"
 #include "virthread.h"
 #include "count-one-bits.h"
@@ -61,13 +62,16 @@ VIR_LOG_INIT("util.random");
 uint64_t virRandomBits(int nbits)
 {
     uint64_t ret = 0;
+    uint8_t val[8];
 
-    if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) {
+    if (virRandomBytes((unsigned char *) &val, sizeof(val)) < 0) {
         /* You're already hosed, so this particular non-random value
          * isn't any worse.  */
         return 0;
     }
 
+    ret = virReadBufInt64LE(val);
+
     if (nbits < 64)
         ret &= (1ULL << nbits) - 1;
 
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe
Posted by Pino Toscano 5 years, 7 months ago
On Thursday, 2 August 2018 09:56:49 CEST Bjoern Walk wrote:
> Make the generation of random bits in virRandomBits independent of the
> endianness of the running architecture.
> 
> This also solves problems with the mocked random byte generation on
> big-endian machines.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com>
> ---
> This goes on top of Michal's fix: https://www.redhat.com/archives/libvir-list/2018-August/msg00080.html
> 
>  src/util/virrandom.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/util/virrandom.c b/src/util/virrandom.c
> index 7915f653..26ff68f5 100644
> --- a/src/util/virrandom.c
> +++ b/src/util/virrandom.c
> @@ -34,6 +34,7 @@
>  # include <gnutls/crypto.h>
>  #endif
>  
> +#include "virendian.h"
>  #include "virrandom.h"
>  #include "virthread.h"
>  #include "count-one-bits.h"
> @@ -61,13 +62,16 @@ VIR_LOG_INIT("util.random");
>  uint64_t virRandomBits(int nbits)
>  {
>      uint64_t ret = 0;
> +    uint8_t val[8];
>  
> -    if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) {
> +    if (virRandomBytes((unsigned char *) &val, sizeof(val)) < 0) {
>          /* You're already hosed, so this particular non-random value
>           * isn't any worse.  */
>          return 0;
>      }
>  
> +    ret = virReadBufInt64LE(val);
> +

I do not think this patch is correct: we are dealing with random bytes,
so there is no "endianness" for them.

-- 
Pino Toscano--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe
Posted by Bjoern Walk 5 years, 7 months ago
Pino Toscano <ptoscano@redhat.com> [2018-08-02, 10:02AM +0200]:
> I do not think this patch is correct: we are dealing with random bytes,
> so there is no "endianness" for them.

Well, it's not incorrect either, isn't it? I agree that endianness
doesn't matter for random data, but in the same time, it doesn't hurt to
have the same random data generated on big- and little-endian machines.
And it gives an easy and future-proof fix for the mocked tests.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Thu, Aug 02, 2018 at 10:17:32AM +0200, Bjoern Walk wrote:
> Pino Toscano <ptoscano@redhat.com> [2018-08-02, 10:02AM +0200]:
> > I do not think this patch is correct: we are dealing with random bytes,
> > so there is no "endianness" for them.
> 
> Well, it's not incorrect either, isn't it? I agree that endianness
> doesn't matter for random data, but in the same time, it doesn't hurt to
> have the same random data generated on big- and little-endian machines.
> And it gives an easy and future-proof fix for the mocked tests.

Lets just modijfy tests/virrandommock.c to add mocking of virRandomBits
alongside virRandomBytes.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe
Posted by Pino Toscano 5 years, 7 months ago
On Thursday, 2 August 2018 12:28:45 CEST Daniel P. Berrangé wrote:
> On Thu, Aug 02, 2018 at 10:17:32AM +0200, Bjoern Walk wrote:
> > Pino Toscano <ptoscano@redhat.com> [2018-08-02, 10:02AM +0200]:
> > > I do not think this patch is correct: we are dealing with random bytes,
> > > so there is no "endianness" for them.
> > 
> > Well, it's not incorrect either, isn't it? I agree that endianness
> > doesn't matter for random data, but in the same time, it doesn't hurt to
> > have the same random data generated on big- and little-endian machines.

Not sure I understand -- since it's random data, you cannot have it
"the same", no matter which endianness the machine has.

> > And it gives an easy and future-proof fix for the mocked tests.

IMHO every mocked test has its own behaviour, and thus the mocking
needs to reflect that.

> Lets just modijfy tests/virrandommock.c to add mocking of virRandomBits
> alongside virRandomBytes.

I don't see how it will help, since all virRandomBits does is calling
virRandomBytes.

I still did not see any complaints about my patch to fix viriscsitest
(since the problem is specific for it), what about ACKing it then?

-- 
Pino Toscano--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Thu, Aug 02, 2018 at 01:12:02PM +0200, Pino Toscano wrote:
> On Thursday, 2 August 2018 12:28:45 CEST Daniel P. Berrangé wrote:
> > On Thu, Aug 02, 2018 at 10:17:32AM +0200, Bjoern Walk wrote:
> > > Pino Toscano <ptoscano@redhat.com> [2018-08-02, 10:02AM +0200]:
> > > > I do not think this patch is correct: we are dealing with random bytes,
> > > > so there is no "endianness" for them.
> > > 
> > > Well, it's not incorrect either, isn't it? I agree that endianness
> > > doesn't matter for random data, but in the same time, it doesn't hurt to
> > > have the same random data generated on big- and little-endian machines.
> 
> Not sure I understand -- since it's random data, you cannot have it
> "the same", no matter which endianness the machine has.
> 
> > > And it gives an easy and future-proof fix for the mocked tests.
> 
> IMHO every mocked test has its own behaviour, and thus the mocking
> needs to reflect that.
> 
> > Lets just modijfy tests/virrandommock.c to add mocking of virRandomBits
> > alongside virRandomBytes.
> 
> I don't see how it will help, since all virRandomBits does is calling
> virRandomBytes.

That's exactly what it would fix. A virRandomBits mock would simply
return a fixed number and not call the mocked virRandomBytes at all
thus avoiding the endianness problem

> I still did not see any complaints about my patch to fix viriscsitest
> (since the problem is specific for it), what about ACKing it then?

The virrandommock.c was intended to provide "stable" random numbers across
multiple tests. It didn't mock virRandomBits because it was mistakenly
assumed that mocking virRandomBytes was enough. Changing virscsitest just
fixes the one occurrance that happens to hit today - we want to fix the
general problem as virrandommock.c intended.


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list