| Message ID | 20190818011329.14499-6-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Sun, Aug 18, 2019 at 04:13:20AM +0300, Laurent Pinchart wrote: > Commit 92b4af98cd67 ("test: Add EventNotifier thread move test") causes > the build to fail in the Chromium OS build environment, because the > return values of the pipe() function marked with the > __warn_unused_result__ attribute is ignored. Fix this. > Do we want to compile tests on CrOS? > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > test/event-thread.cpp | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/test/event-thread.cpp b/test/event-thread.cpp > index 714bc9845820..01120733eca4 100644 > --- a/test/event-thread.cpp > +++ b/test/event-thread.cpp > @@ -25,7 +25,11 @@ public: > EventHandler() > : notified_(false) > { > - pipe(pipefd_); > + int ret = pipe(pipefd_); > + if (ret < 0) { > + ret = errno; > + cout << "pipe() failed: " << strerror(ret) << endl; > + } Shouldn't we fail the test if creating the pipe fails? This will require moving this out of the constructor probably... > > notifier_ = new EventNotifier(pipefd_[0], EventNotifier::Read, this); > notifier_->activated.connect(this, &EventHandler::readReady); > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Sun, Aug 18, 2019 at 07:24:58PM +0200, Jacopo Mondi wrote: > On Sun, Aug 18, 2019 at 04:13:20AM +0300, Laurent Pinchart wrote: > > Commit 92b4af98cd67 ("test: Add EventNotifier thread move test") causes > > the build to fail in the Chromium OS build environment, because the > > return values of the pipe() function marked with the > > __warn_unused_result__ attribute is ignored. Fix this. > > > > Do we want to compile tests on CrOS? I thought we didn't, and went investigating. Turns out there's a typo in the ebuild, so tests are always compiled. I'll fix that. It's a good test though :-) > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > test/event-thread.cpp | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/test/event-thread.cpp b/test/event-thread.cpp > > index 714bc9845820..01120733eca4 100644 > > --- a/test/event-thread.cpp > > +++ b/test/event-thread.cpp > > @@ -25,7 +25,11 @@ public: > > EventHandler() > > : notified_(false) > > { > > - pipe(pipefd_); > > + int ret = pipe(pipefd_); > > + if (ret < 0) { > > + ret = errno; > > + cout << "pipe() failed: " << strerror(ret) << endl; > > + } > > Shouldn't we fail the test if creating the pipe fails? > This will require moving this out of the constructor probably... The test will eventually fail as the event will not be notified, so I think we're good. > > notifier_ = new EventNotifier(pipefd_[0], EventNotifier::Read, this); > > notifier_->activated.connect(this, &EventHandler::readReady);
On Sun, Aug 18, 2019 at 04:13:20AM +0300, Laurent Pinchart wrote: > Commit 92b4af98cd67 ("test: Add EventNotifier thread move test") causes > the build to fail in the Chromium OS build environment, because the > return values of the pipe() function marked with the > __warn_unused_result__ attribute is ignored. Fix this. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> It fixed the compilation for me :) Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > test/event-thread.cpp | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/test/event-thread.cpp b/test/event-thread.cpp > index 714bc9845820..01120733eca4 100644 > --- a/test/event-thread.cpp > +++ b/test/event-thread.cpp > @@ -25,7 +25,11 @@ public: > EventHandler() > : notified_(false) > { > - pipe(pipefd_); > + int ret = pipe(pipefd_); > + if (ret < 0) { > + ret = errno; > + cout << "pipe() failed: " << strerror(ret) << endl; > + } > > notifier_ = new EventNotifier(pipefd_[0], EventNotifier::Read, this); > notifier_->activated.connect(this, &EventHandler::readReady); > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Sun, Aug 18, 2019 at 08:26:36PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Sun, Aug 18, 2019 at 07:24:58PM +0200, Jacopo Mondi wrote: > > On Sun, Aug 18, 2019 at 04:13:20AM +0300, Laurent Pinchart wrote: > > > Commit 92b4af98cd67 ("test: Add EventNotifier thread move test") causes > > > the build to fail in the Chromium OS build environment, because the > > > return values of the pipe() function marked with the > > > __warn_unused_result__ attribute is ignored. Fix this. > > > > > > > Do we want to compile tests on CrOS? > > I thought we didn't, and went investigating. Turns out there's a typo in > the ebuild, so tests are always compiled. I'll fix that. It's a good > test though :-) > And do we want to keep building tests on cros ? I would say why not, actually... It already spotted something > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > test/event-thread.cpp | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/test/event-thread.cpp b/test/event-thread.cpp > > > index 714bc9845820..01120733eca4 100644 > > > --- a/test/event-thread.cpp > > > +++ b/test/event-thread.cpp > > > @@ -25,7 +25,11 @@ public: > > > EventHandler() > > > : notified_(false) > > > { > > > - pipe(pipefd_); > > > + int ret = pipe(pipefd_); > > > + if (ret < 0) { > > > + ret = errno; > > > + cout << "pipe() failed: " << strerror(ret) << endl; > > > + } > > > > Shouldn't we fail the test if creating the pipe fails? > > This will require moving this out of the constructor probably... > > The test will eventually fail as the event will not be notified, so I > think we're good. > Thanks for the clarification. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > > notifier_ = new EventNotifier(pipefd_[0], EventNotifier::Read, this); > > > notifier_->activated.connect(this, &EventHandler::readReady); > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Aug 19, 2019 at 06:03:04PM +0200, Jacopo Mondi wrote: > On Sun, Aug 18, 2019 at 08:26:36PM +0300, Laurent Pinchart wrote: > > On Sun, Aug 18, 2019 at 07:24:58PM +0200, Jacopo Mondi wrote: > >> On Sun, Aug 18, 2019 at 04:13:20AM +0300, Laurent Pinchart wrote: > >>> Commit 92b4af98cd67 ("test: Add EventNotifier thread move test") causes > >>> the build to fail in the Chromium OS build environment, because the > >>> return values of the pipe() function marked with the > >>> __warn_unused_result__ attribute is ignored. Fix this. > >>> > >> > >> Do we want to compile tests on CrOS? > > > > I thought we didn't, and went investigating. Turns out there's a typo in > > the ebuild, so tests are always compiled. I'll fix that. It's a good > > test though :-) > > And do we want to keep building tests on cros ? > I would say why not, actually... It already spotted something Yes, they're useful for this purpose :-) > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> test/event-thread.cpp | 6 +++++- > >>> 1 file changed, 5 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/test/event-thread.cpp b/test/event-thread.cpp > >>> index 714bc9845820..01120733eca4 100644 > >>> --- a/test/event-thread.cpp > >>> +++ b/test/event-thread.cpp > >>> @@ -25,7 +25,11 @@ public: > >>> EventHandler() > >>> : notified_(false) > >>> { > >>> - pipe(pipefd_); > >>> + int ret = pipe(pipefd_); > >>> + if (ret < 0) { > >>> + ret = errno; > >>> + cout << "pipe() failed: " << strerror(ret) << endl; > >>> + } > >> > >> Shouldn't we fail the test if creating the pipe fails? > >> This will require moving this out of the constructor probably... > > > > The test will eventually fail as the event will not be notified, so I > > think we're good. > > Thanks for the clarification. > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > >>> notifier_ = new EventNotifier(pipefd_[0], EventNotifier::Read, this); > >>> notifier_->activated.connect(this, &EventHandler::readReady);
diff --git a/test/event-thread.cpp b/test/event-thread.cpp index 714bc9845820..01120733eca4 100644 --- a/test/event-thread.cpp +++ b/test/event-thread.cpp @@ -25,7 +25,11 @@ public: EventHandler() : notified_(false) { - pipe(pipefd_); + int ret = pipe(pipefd_); + if (ret < 0) { + ret = errno; + cout << "pipe() failed: " << strerror(ret) << endl; + } notifier_ = new EventNotifier(pipefd_[0], EventNotifier::Read, this); notifier_->activated.connect(this, &EventHandler::readReady);
Commit 92b4af98cd67 ("test: Add EventNotifier thread move test") causes the build to fail in the Chromium OS build environment, because the return values of the pipe() function marked with the __warn_unused_result__ attribute is ignored. Fix this. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- test/event-thread.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)