[LTP] [PATCH 03/21] test.h/tst_brkm: Drop __attribute__((noreturn)) since tst_brk can return to caller

[LTP] [PATCH 03/21] test.h/tst_brkm: Drop __attribute__((noreturn)) since tst_brk can return to caller

[Markos Chandras]

[-- Attachment #1: Type: text/plain, Size: 49 bytes --]

Hi, please review the attached patch

-- 
markos

[-- Attachment #2: 0003-test.h-tst_brkm-Drop-__attribute__-noreturn-since-ts.patch --]
[-- Type: text/plain, Size: 1011 bytes --]

From ab2d6650577acb973f68ddfd8884fab0116e5470 Mon Sep 17 00:00:00 2001
From: Markos Chandras <markos.chandras@imgtec.com>
Date: Thu, 22 Dec 2011 11:42:10 +0000
Subject: [PATCH 03/21] test.h/tst_brkm: Drop __attribute__((noreturn)) since tst_brk can return to caller

Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 include/test.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/test.h b/include/test.h
index 033fdbc..39bad72 100644
--- a/include/test.h
+++ b/include/test.h
@@ -189,7 +189,7 @@ void tst_resm(int ttype, char *arg_fmt, ...)
 void tst_brk(int ttype, char *fname, void (*func)(void), char *arg_fmt, ...)
 	__attribute__ ((format (printf, 4, 5)));
 void tst_brkm(int ttype, void (*func)(void), char *arg_fmt, ...)
-	__attribute__ ((format (printf, 3, 4))) LTP_ATTRIBUTE_NORETURN;
+	__attribute__ ((format (printf, 3, 4)));
 void tst_require_root(void (*func)(void));
 int  tst_environ(void);
 void tst_exit(void) LTP_ATTRIBUTE_NORETURN;
-- 
1.7.1


[-- Attachment #3: Type: text/plain, Size: 355 bytes --]

------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

[-- Attachment #4: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH 03/21] test.h/tst_brkm: Drop __attribute__((noreturn)) since tst_brk can return to caller

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 481 bytes --]

On Tuesday 03 January 2012 04:37:10 Markos Chandras wrote:
> Hi, please review the attached patch

the tst_brk() API has gotten annoying.  it is designed to not return to the 
original caller which is why it had the "noreturn" attribute.  but Garret re-
rewrote it to be re-entrant presumably so the cleanup() func could itself call 
tst_brk().  but i'm not sure how useful that actually is in practice ...

Garret: could you comment on the actual use cases here ?
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 355 bytes --]

------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH 03/21] test.h/tst_brkm: Drop __attribute__((noreturn)) since tst_brk can return to caller

[Cyril Hrubis]

Hi!
> the tst_brk() API has gotten annoying.  it is designed to not return to the 
> original caller which is why it had the "noreturn" attribute.  but Garret re-
> rewrote it to be re-entrant presumably so the cleanup() func could itself call 
> tst_brk().  but i'm not sure how useful that actually is in practice ...

The original problem here is that tests use tst_brkm() in cleanup
functions where you don't want to exit before you do all the cleanups.
That was previously working as tst_brkm() with cleanup set to NULL
wasn't calling tst_exit() (and obviously you would never add cleanup
pointer to tst_brkm() when you are in cleanup function otherwise you are
in infinite loop).

The original behavior was IMHO wrong and now it's little better. And I
think that best solution would be to fix tests to use tst_resm() instead
of tst_brkm() when the test doesn't want to exit immediately.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Ridiculously easy VDI. With Citrix VDI-in-a-Box, you don't need a complex
infrastructure or vast IT resources to deliver seamless, secure access to
virtual desktops. With this all-in-one solution, easily deploy virtual 
desktops for less than the cost of PCs and save 60% on VDI infrastructure 
costs. Try it free! http://p.sf.net/sfu/Citrix-VDIinabox
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH 03/21] test.h/tst_brkm: Drop __attribute__((noreturn)) since tst_brk can return to caller

[Cyril Hrubis]

Hi!
> 1. Create consistent code that matched what we wanted as far as style
> and structure was concerned. A lot of the time Cyril and I (as well as
> you and other folks in the past) were making modifications to files
> and instead of it being just a "oh, I fixed this thing", it generally
> turned into a full-scale rewrite of the testcase because we weren't
> very restrained in how changes were done. Furthermore, without there
> being better examples in the tree for developers to look at, it was
> believed (at least by me) that people were following the wrong
> examples when designing fixes and the like, and regardless of what the
> style guide said, or whether or not they were told to run
> check_patches.pl, contributors provided code that be more of a burden
> in the long run. This also extended to proper design patterns as noted
> in the style guide.

Yes we have a problem with very unconsistent usage of the test
interfaces but I don't see any other way than taking and fixing these
tests one by one.

And for that we need clear definitions and docummentation how to use the
test interface. The style guide is good, but I'm thinking about
something more detailed with actuall code examples.

I would love to write such documment but so far I haven't had time for
it.

> 2. Create a means where people could call "SAFE_*" macros instead of
> doing ad hoc error checking (there was a lot of duplication in the LTP
> tree, and there were a lot of gaps because test writers didn't write
> their error codes and things would go downhill quickly and be hard to
> troubleshoot if the issue was transient). In order to do this, we
> needed a means to push 'Exceptions' on a stack and execute all of the
> cleanup functions in order to ensure that everything was completed to
> the best of the ability of the testcase. In python terms what I was
> looking for was something like the following (basic pseudocode,
> similar to what's done in unittest):
> 
> setup()
> try:
>     test()
> finally:
>     # Execute all of the cleanup tasks in order. If something fatal
> occurs here, die.
>     map(lambda cleanup_task: cleanup_task(), cleanup_tasks.iteritems())
> 
> In order to do this, tst_brkm needed to become reentrant to avoid
> rewriting everything to use a new API.
> 
> Like all things, I'm not 100% pleased with how things turned out
> (things aren't cleanly formulated in the C code, similar to what I
> show above in the python psuedocode), but if you have some suggestions
> for how we should resolve this in a sane manner (maybe grow another
> API that's reentrant, adjust the behavior of the current one to once
> again be non-reentrant, etc?) -- I'm all ears.

Growing parallel API is not a good option, that way we would have to
maintain two broken interfaces ;).

My only recipe for that is gradually fix tests one by one (And I'm
working on that as fast as I can).

> PS This work burned me out on LTP as I took on the tedious task of
> changing a couple thousands of lines of code in all the testcases in
> the period of a couple weeks (it was sometime between Thanksgiving and
> somewhere around New Year's), originally by hand, then I tried to do
> via script, then I went back and had to manually fix things the script
> didn't catch the first go around. This was reiterated a few times. And
> I didn't catch all the bugs (I've sighed in regret at a few commit
> messages I've seen over the past year caused by my rototilling).

Hey, that's not a big deal, so far I've managed to fix most of the
problems caused by that. Mostly it caused broken things to broke even
more and things that accidentally stopped working after they were
accidentally working before.

The git history is another story, but at least it's challenging and fun
;).

And one thing that I've learnt while maintaining LTP is that it's ok to
try things and break something occasionally if you fix that later. If
you are too afraid you wouldn't be able to do anything. 

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Ridiculously easy VDI. With Citrix VDI-in-a-Box, you don't need a complex
infrastructure or vast IT resources to deliver seamless, secure access to
virtual desktops. With this all-in-one solution, easily deploy virtual 
desktops for less than the cost of PCs and save 60% on VDI infrastructure 
costs. Try it free! http://p.sf.net/sfu/Citrix-VDIinabox
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list