[libgpiod,5/5] bindings: cxx: Workaround --success run
diff mbox series

Message ID 20190807195132.7538-5-alexander.stein@mailbox.org
State New
Headers show
Series
  • [libgpiod,1/5] bindings: cxx: Use 'upstream' include path
Related show

Commit Message

Alexander Stein Aug. 7, 2019, 7:51 p.m. UTC
If run with --success, all expressions are evaluated and printed out.
But REQUIRE_FALSE(chip) tries to iterate over the chip resulting in this
backtrace:
#0  gpiod_chip_num_lines (chip=chip@entry=0x0) at ../../lib/core.c:235
#1  gpiod_line_iter_new (chip=0x0) at ../../lib/iter.c:140
#2  gpiod::(anonymous namespace)::make_line_iter (chip=0x0) at ../../../bindings/cxx/iter.cpp:29
#3  gpiod::line_iter::line_iter (this=0x7fffffffd690, owner=...) at ../../../bindings/cxx/iter.cpp:109
#4  Catch::rangeToString<gpiod::chip> (range=...) at /usr/include/catch2/catch.hpp:1959
[...]

Workaround by forcing catch2 to call gpiod::chip::operator bool().

Signed-off-by: Alexander Stein <alexander.stein@mailbox.org>
---
This actually looks like a flaw in the binding itself that the
gpiod::line_iter can't cope with an empty gpiod::chip.

 bindings/cxx/tests/tests-chip.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Bartosz Golaszewski Aug. 8, 2019, 3:27 p.m. UTC | #1
śr., 7 sie 2019 o 21:51 Alexander Stein <alexander.stein@mailbox.org>
napisał(a):
>
> If run with --success, all expressions are evaluated and printed out.
> But REQUIRE_FALSE(chip) tries to iterate over the chip resulting in this
> backtrace:
> #0  gpiod_chip_num_lines (chip=chip@entry=0x0) at ../../lib/core.c:235
> #1  gpiod_line_iter_new (chip=0x0) at ../../lib/iter.c:140
> #2  gpiod::(anonymous namespace)::make_line_iter (chip=0x0) at ../../../bindings/cxx/iter.cpp:29
> #3  gpiod::line_iter::line_iter (this=0x7fffffffd690, owner=...) at ../../../bindings/cxx/iter.cpp:109
> #4  Catch::rangeToString<gpiod::chip> (range=...) at /usr/include/catch2/catch.hpp:1959
> [...]
>
> Workaround by forcing catch2 to call gpiod::chip::operator bool().
>
> Signed-off-by: Alexander Stein <alexander.stein@mailbox.org>
> ---
> This actually looks like a flaw in the binding itself that the
> gpiod::line_iter can't cope with an empty gpiod::chip.
>

Do you want to submit a patch that fixes that? Otherwise I can fix it.
I think that simply throwing an exception on empty chip is enough,
right?

Bart

>  bindings/cxx/tests/tests-chip.cpp | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/bindings/cxx/tests/tests-chip.cpp b/bindings/cxx/tests/tests-chip.cpp
> index c9eb8e5..1c69872 100644
> --- a/bindings/cxx/tests/tests-chip.cpp
> +++ b/bindings/cxx/tests/tests-chip.cpp
> @@ -70,7 +70,7 @@ TEST_CASE("GPIO chip can be opened with the open() method in different modes", "
>         mockup::probe_guard mockup_chips({ 8, 8, 8 });
>         ::gpiod::chip chip;
>
> -       REQUIRE_FALSE(chip);
> +       REQUIRE_FALSE(!!chip);
>
>         SECTION("open by name")
>         {
> @@ -102,7 +102,7 @@ TEST_CASE("Uninitialized GPIO chip behaves correctly", "[chip]")
>
>         SECTION("uninitialized chip is 'false'")
>         {
> -               REQUIRE_FALSE(chip);
> +               REQUIRE_FALSE(!!chip);
>         }
>
>         SECTION("using uninitialized chip throws logic_error")
> @@ -149,7 +149,7 @@ TEST_CASE("Chip object can be reset", "[chip]")
>         ::gpiod::chip chip(mockup::instance().chip_name(0));
>         REQUIRE(chip);
>         chip.reset();
> -       REQUIRE_FALSE(chip);
> +       REQUIRE_FALSE(!!chip);
>  }
>
>  TEST_CASE("Chip info can be correctly retrieved", "[chip]")
> --
> 2.22.0
>
Alexander Stein Aug. 8, 2019, 6:41 p.m. UTC | #2
On Thursday, August 8, 2019, 5:27:14 PM CEST Bartosz Golaszewski wrote:
> śr., 7 sie 2019 o 21:51 Alexander Stein <alexander.stein@mailbox.org>
> napisał(a):
> >
> > If run with --success, all expressions are evaluated and printed out.
> > But REQUIRE_FALSE(chip) tries to iterate over the chip resulting in this
> > backtrace:
> > #0  gpiod_chip_num_lines (chip=chip@entry=0x0) at ../../lib/core.c:235
> > #1  gpiod_line_iter_new (chip=0x0) at ../../lib/iter.c:140
> > #2  gpiod::(anonymous namespace)::make_line_iter (chip=0x0) at ../../../bindings/cxx/iter.cpp:29
> > #3  gpiod::line_iter::line_iter (this=0x7fffffffd690, owner=...) at ../../../bindings/cxx/iter.cpp:109
> > #4  Catch::rangeToString<gpiod::chip> (range=...) at /usr/include/catch2/catch.hpp:1959
> > [...]
> >
> > Workaround by forcing catch2 to call gpiod::chip::operator bool().
> >
> > Signed-off-by: Alexander Stein <alexander.stein@mailbox.org>
> > ---
> > This actually looks like a flaw in the binding itself that the
> > gpiod::line_iter can't cope with an empty gpiod::chip.
> >
> 
> Do you want to submit a patch that fixes that? Otherwise I can fix it.
> I think that simply throwing an exception on empty chip is enough,
> right?

Reading that backtrace today, the actual problem is gpiod_chip_num_lines deferencing the nullptr.
There are 2 possibilities:
* if gpiod_chip is NULL in gpiod_line_iter_new(), return NULLL iter as well (which will raise an exception on line iter.cpp:31)
* return an iter with num_lines = 0

Can't rate the 2nd one if this will raise other problems.

Best regards,
Alexander
Bartosz Golaszewski Aug. 9, 2019, 6:55 a.m. UTC | #3
czw., 8 sie 2019 o 20:41 Alexander Stein <alexander.stein@mailbox.org>
napisał(a):
>
>  On Thursday, August 8, 2019, 5:27:14 PM CEST Bartosz Golaszewski wrote:
> > śr., 7 sie 2019 o 21:51 Alexander Stein <alexander.stein@mailbox.org>
> > napisał(a):
> > >
> > > If run with --success, all expressions are evaluated and printed out.
> > > But REQUIRE_FALSE(chip) tries to iterate over the chip resulting in this
> > > backtrace:
> > > #0  gpiod_chip_num_lines (chip=chip@entry=0x0) at ../../lib/core.c:235
> > > #1  gpiod_line_iter_new (chip=0x0) at ../../lib/iter.c:140
> > > #2  gpiod::(anonymous namespace)::make_line_iter (chip=0x0) at ../../../bindings/cxx/iter.cpp:29
> > > #3  gpiod::line_iter::line_iter (this=0x7fffffffd690, owner=...) at ../../../bindings/cxx/iter.cpp:109
> > > #4  Catch::rangeToString<gpiod::chip> (range=...) at /usr/include/catch2/catch.hpp:1959
> > > [...]
> > >
> > > Workaround by forcing catch2 to call gpiod::chip::operator bool().
> > >
> > > Signed-off-by: Alexander Stein <alexander.stein@mailbox.org>
> > > ---
> > > This actually looks like a flaw in the binding itself that the
> > > gpiod::line_iter can't cope with an empty gpiod::chip.
> > >
> >
> > Do you want to submit a patch that fixes that? Otherwise I can fix it.
> > I think that simply throwing an exception on empty chip is enough,
> > right?
>
> Reading that backtrace today, the actual problem is gpiod_chip_num_lines deferencing the nullptr.
> There are 2 possibilities:
> * if gpiod_chip is NULL in gpiod_line_iter_new(), return NULLL iter as well (which will raise an exception on line iter.cpp:31)
> * return an iter with num_lines = 0
>
> Can't rate the 2nd one if this will raise other problems.
>
> Best regards,
> Alexander
>
>
>

Hardening of argument validation in the core library (checking of
NULL-pointers, etc.) has been on my TODO list for some time now. It
seems I'll finally have to do it.

Thanks for the patches, all applied!

Bartosz

Patch
diff mbox series

diff --git a/bindings/cxx/tests/tests-chip.cpp b/bindings/cxx/tests/tests-chip.cpp
index c9eb8e5..1c69872 100644
--- a/bindings/cxx/tests/tests-chip.cpp
+++ b/bindings/cxx/tests/tests-chip.cpp
@@ -70,7 +70,7 @@  TEST_CASE("GPIO chip can be opened with the open() method in different modes", "
 	mockup::probe_guard mockup_chips({ 8, 8, 8 });
 	::gpiod::chip chip;
 
-	REQUIRE_FALSE(chip);
+	REQUIRE_FALSE(!!chip);
 
 	SECTION("open by name")
 	{
@@ -102,7 +102,7 @@  TEST_CASE("Uninitialized GPIO chip behaves correctly", "[chip]")
 
 	SECTION("uninitialized chip is 'false'")
 	{
-		REQUIRE_FALSE(chip);
+		REQUIRE_FALSE(!!chip);
 	}
 
 	SECTION("using uninitialized chip throws logic_error")
@@ -149,7 +149,7 @@  TEST_CASE("Chip object can be reset", "[chip]")
 	::gpiod::chip chip(mockup::instance().chip_name(0));
 	REQUIRE(chip);
 	chip.reset();
-	REQUIRE_FALSE(chip);
+	REQUIRE_FALSE(!!chip);
 }
 
 TEST_CASE("Chip info can be correctly retrieved", "[chip]")