| Message ID | 20220316060214.2200695-1-morbo@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] libfdt: use logical "or" instead of bitwise "or" with boolean operands | expand |
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
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
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 >
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
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
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
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);
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(-)