[kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands

[kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands

[Bill Wendling]

Clang warns about using a bitwise '|' with boolean operands. This seems
to be due to a small typo.

  lib/libfdt/fdt_rw.c:438:6: warning: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
          if (can_assume(LIBFDT_ORDER) |

Using '||' removes this warnings.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/libfdt/fdt_rw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c
index 13854253ff86..3320e5559cac 100644
--- a/lib/libfdt/fdt_rw.c
+++ b/lib/libfdt/fdt_rw.c
@@ -435,7 +435,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
 			return struct_size;
 	}
 
-	if (can_assume(LIBFDT_ORDER) |
+	if (can_assume(LIBFDT_ORDER) ||
 	    !fdt_blocks_misordered_(fdt, mem_rsv_size, struct_size)) {
 		/* no further work necessary */
 		err = fdt_move(fdt, buf, bufsize);
-- 
2.35.1.723.g4982287a31-goog

Re: [kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands

[Andrew Jones]

On Tue, Mar 15, 2022 at 11:02:14PM -0700, Bill Wendling wrote:
> Clang warns about using a bitwise '|' with boolean operands. This seems
> to be due to a small typo.
> 
>   lib/libfdt/fdt_rw.c:438:6: warning: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
>           if (can_assume(LIBFDT_ORDER) |
> 
> Using '||' removes this warnings.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>  lib/libfdt/fdt_rw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c
> index 13854253ff86..3320e5559cac 100644
> --- a/lib/libfdt/fdt_rw.c
> +++ b/lib/libfdt/fdt_rw.c
> @@ -435,7 +435,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
>  			return struct_size;
>  	}
>  
> -	if (can_assume(LIBFDT_ORDER) |
> +	if (can_assume(LIBFDT_ORDER) ||
>  	    !fdt_blocks_misordered_(fdt, mem_rsv_size, struct_size)) {
>  		/* no further work necessary */
>  		err = fdt_move(fdt, buf, bufsize);
> -- 
> 2.35.1.723.g4982287a31-goog
>

This is fixed in libfdt upstream with commit 7be250b4 ("libfdt:
Correct condition for reordering blocks"), which is in v1.6.1.
We can either take this patch as a backport of 7be250b4 or we
can rebase all of our libfdt to v1.6.1. Based on the number of
fixes in v1.6.1, which appear to be mostly for compiling with
later compilers, I'm in favor of rebasing.

Actually, we can also use this opportunity to [re]visit the
idea of changing libfdt to a git submodule. I'd like to hear
opinions on that.

Thanks,
drew

Re: [kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands

[Thomas Huth]

On 16/03/2022 08.43, Andrew Jones wrote:
> On Tue, Mar 15, 2022 at 11:02:14PM -0700, Bill Wendling wrote:
>> Clang warns about using a bitwise '|' with boolean operands. This seems
>> to be due to a small typo.
>>
>>    lib/libfdt/fdt_rw.c:438:6: warning: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
>>            if (can_assume(LIBFDT_ORDER) |
>>
>> Using '||' removes this warnings.
>>
>> Signed-off-by: Bill Wendling <morbo@google.com>
>> ---
>>   lib/libfdt/fdt_rw.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c
>> index 13854253ff86..3320e5559cac 100644
>> --- a/lib/libfdt/fdt_rw.c
>> +++ b/lib/libfdt/fdt_rw.c
>> @@ -435,7 +435,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
>>   			return struct_size;
>>   	}
>>   
>> -	if (can_assume(LIBFDT_ORDER) |
>> +	if (can_assume(LIBFDT_ORDER) ||
>>   	    !fdt_blocks_misordered_(fdt, mem_rsv_size, struct_size)) {
>>   		/* no further work necessary */
>>   		err = fdt_move(fdt, buf, bufsize);
>> -- 
>> 2.35.1.723.g4982287a31-goog
>>
> 
> This is fixed in libfdt upstream with commit 7be250b4 ("libfdt:
> Correct condition for reordering blocks"), which is in v1.6.1.
> We can either take this patch as a backport of 7be250b4 or we
> can rebase all of our libfdt to v1.6.1. Based on the number of
> fixes in v1.6.1, which appear to be mostly for compiling with
> later compilers, I'm in favor of rebasing.

+1 for updating to v1.6.1 completely.

> Actually, we can also use this opportunity to [re]visit the
> idea of changing libfdt to a git submodule. I'd like to hear
> opinions on that.

I'm always a little bit torn when it comes to this question. Sure, git 
submodules maybe make the update easier ... but they are a real pita when 
you're working with remote machines that might not have direct connection to 
the internet. For example, I'm used to rsync my sources to the non-x86 
machines, and if you forget to update the submodule to the right state 
before the sync, you've just lost. So from my side, it's a preference for 
continuing without submodules (but I won't insist if everybody else wants to 
have them instead).

  Thomas

Re: [kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands

[Bill Wendling]

On Wed, Mar 16, 2022 at 12:53 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 16/03/2022 08.43, Andrew Jones wrote:
> > On Tue, Mar 15, 2022 at 11:02:14PM -0700, Bill Wendling wrote:
> >> Clang warns about using a bitwise '|' with boolean operands. This seems
> >> to be due to a small typo.
> >>
> >>    lib/libfdt/fdt_rw.c:438:6: warning: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> >>            if (can_assume(LIBFDT_ORDER) |
> >>
> >> Using '||' removes this warnings.
> >>
> >> Signed-off-by: Bill Wendling <morbo@google.com>
> >> ---
> >>   lib/libfdt/fdt_rw.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c
> >> index 13854253ff86..3320e5559cac 100644
> >> --- a/lib/libfdt/fdt_rw.c
> >> +++ b/lib/libfdt/fdt_rw.c
> >> @@ -435,7 +435,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
> >>                      return struct_size;
> >>      }
> >>
> >> -    if (can_assume(LIBFDT_ORDER) |
> >> +    if (can_assume(LIBFDT_ORDER) ||
> >>          !fdt_blocks_misordered_(fdt, mem_rsv_size, struct_size)) {
> >>              /* no further work necessary */
> >>              err = fdt_move(fdt, buf, bufsize);
> >> --
> >> 2.35.1.723.g4982287a31-goog
> >>
> >
> > This is fixed in libfdt upstream with commit 7be250b4 ("libfdt:
> > Correct condition for reordering blocks"), which is in v1.6.1.
> > We can either take this patch as a backport of 7be250b4 or we
> > can rebase all of our libfdt to v1.6.1. Based on the number of
> > fixes in v1.6.1, which appear to be mostly for compiling with
> > later compilers, I'm in favor of rebasing.
>
> +1 for updating to v1.6.1 completely.
>
Also +1. :-) Thank you!

-bw

> > Actually, we can also use this opportunity to [re]visit the
> > idea of changing libfdt to a git submodule. I'd like to hear
> > opinions on that.
>
> I'm always a little bit torn when it comes to this question. Sure, git
> submodules maybe make the update easier ... but they are a real pita when
> you're working with remote machines that might not have direct connection to
> the internet. For example, I'm used to rsync my sources to the non-x86
> machines, and if you forget to update the submodule to the right state
> before the sync, you've just lost. So from my side, it's a preference for
> continuing without submodules (but I won't insist if everybody else wants to
> have them instead).
>
>   Thomas
>

Re: [kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands

[Andrew Jones]

On Tue, Mar 15, 2022 at 11:02:14PM -0700, Bill Wendling wrote:
> Clang warns about using a bitwise '|' with boolean operands. This seems
> to be due to a small typo.
> 
>   lib/libfdt/fdt_rw.c:438:6: warning: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
>           if (can_assume(LIBFDT_ORDER) |
> 
> Using '||' removes this warnings.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>  lib/libfdt/fdt_rw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c
> index 13854253ff86..3320e5559cac 100644
> --- a/lib/libfdt/fdt_rw.c
> +++ b/lib/libfdt/fdt_rw.c
> @@ -435,7 +435,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
>  			return struct_size;
>  	}
>  
> -	if (can_assume(LIBFDT_ORDER) |
> +	if (can_assume(LIBFDT_ORDER) ||
>  	    !fdt_blocks_misordered_(fdt, mem_rsv_size, struct_size)) {
>  		/* no further work necessary */
>  		err = fdt_move(fdt, buf, bufsize);
> -- 
> 2.35.1.723.g4982287a31-goog
>

We're not getting as much interest in the submodule discussion as I hoped.
I see one vote against on this thread and one vote for on a different
thread[1]. For now I'll just commit a big rebase patch for libfdt. We can
revisit it again after we decide what to do for QCBOR.

Thanks,
drew

[1] https://lore.kernel.org/kvm/20220316105109.oi5g532ylijzldte@gator/T/#m48c47c761f3b3a4da636482b6385c59d4a990137 

Re: [kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands

[Andrew Jones]

On Fri, Mar 18, 2022 at 10:36:01AM +0100, Andrew Jones wrote:
> On Tue, Mar 15, 2022 at 11:02:14PM -0700, Bill Wendling wrote:
> > Clang warns about using a bitwise '|' with boolean operands. This seems
> > to be due to a small typo.
> > 
> >   lib/libfdt/fdt_rw.c:438:6: warning: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> >           if (can_assume(LIBFDT_ORDER) |
> > 
> > Using '||' removes this warnings.
> > 
> > Signed-off-by: Bill Wendling <morbo@google.com>
> > ---
> >  lib/libfdt/fdt_rw.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c
> > index 13854253ff86..3320e5559cac 100644
> > --- a/lib/libfdt/fdt_rw.c
> > +++ b/lib/libfdt/fdt_rw.c
> > @@ -435,7 +435,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
> >  			return struct_size;
> >  	}
> >  
> > -	if (can_assume(LIBFDT_ORDER) |
> > +	if (can_assume(LIBFDT_ORDER) ||
> >  	    !fdt_blocks_misordered_(fdt, mem_rsv_size, struct_size)) {
> >  		/* no further work necessary */
> >  		err = fdt_move(fdt, buf, bufsize);
> > -- 
> > 2.35.1.723.g4982287a31-goog
> >
> 
> We're not getting as much interest in the submodule discussion as I hoped.
> I see one vote against on this thread and one vote for on a different
> thread[1]. For now I'll just commit a big rebase patch for libfdt. We can
> revisit it again after we decide what to do for QCBOR.
>

Now merged through misc/queue.

Thanks,
drew 

Re: [kvm-unit-tests PATCH] libfdt: use logical "or" instead of bitwise "or" with boolean operands

[Thomas Huth]

On 18/03/2022 10.36, Andrew Jones wrote:
> On Tue, Mar 15, 2022 at 11:02:14PM -0700, Bill Wendling wrote:
>> Clang warns about using a bitwise '|' with boolean operands. This seems
>> to be due to a small typo.
>>
>>    lib/libfdt/fdt_rw.c:438:6: warning: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
>>            if (can_assume(LIBFDT_ORDER) |
>>
>> Using '||' removes this warnings.
>>
>> Signed-off-by: Bill Wendling <morbo@google.com>
>> ---
>>   lib/libfdt/fdt_rw.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c
>> index 13854253ff86..3320e5559cac 100644
>> --- a/lib/libfdt/fdt_rw.c
>> +++ b/lib/libfdt/fdt_rw.c
>> @@ -435,7 +435,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
>>   			return struct_size;
>>   	}
>>   
>> -	if (can_assume(LIBFDT_ORDER) |
>> +	if (can_assume(LIBFDT_ORDER) ||
>>   	    !fdt_blocks_misordered_(fdt, mem_rsv_size, struct_size)) {
>>   		/* no further work necessary */
>>   		err = fdt_move(fdt, buf, bufsize);
>> -- 
>> 2.35.1.723.g4982287a31-goog
>>
> 
> We're not getting as much interest in the submodule discussion as I hoped.
> I see one vote against on this thread and one vote for on a different
> thread[1]. For now I'll just commit a big rebase patch for libfdt. We can
> revisit it again after we decide what to do for QCBOR.

I recently learnt that there are indeed people who ship kvm-unit-tests with 
their distro - at least buildroot has a package:
https://git.busybox.net/buildroot/tree/package/kvm-unit-tests

So one more argument for copying the files over instead of using submodules: 
The tarballs for tags will be self-contained, e.g.:

https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/archive/v2022-03-08/kvm-unit-tests-v2022-03-08.tar.gz

If we use submodules, I guess the content of the submodule content will be 
missing in there?

  Thomas