All of lore.kernel.org
 help / color / mirror / Atom feed
From: Cyril Hrubis <chrubis@suse.cz>
To: Jan Stancek <jstancek@redhat.com>
Cc: ltp@lists.linux.it
Subject: Re: [LTP] [PATCH] [RFC] lib/tst_test.c: Fix tst_brk() handling
Date: Tue, 26 Nov 2024 15:50:13 +0100	[thread overview]
Message-ID: <Z0XgJVa9czWm2YMc@yuki.lan> (raw)
In-Reply-To: <CAASaF6wWy6HwbBRmTQcYg3WRwhxexE1ZUernL4WKQzOJYeO+Eg@mail.gmail.com>

Hi!
> > This makes the tst_brk() handling cleaner and saner as instead of
> > propagating the tst_brk() result in a return value an abort flag is
> > introduced into the shared memory.
> >
> > Now:
> >
> > - All the processes but the library one that reports the results exit
> >   with 0
> > - tst_brk(TBROK, ...) increments result conters, sets the abort flag.
> >   and exit current process
> > - all other tst_brk() variants will just increments the countes and
> >   exits the current process
> 
> It removes the easy way for parent to check that child hasn't run into
> any issues,
> but I can't recall a specific test we have today that depends on it.

I suppose that we can make the tst_brk flag part of a public API if
anyone needs that but I guess that in the case of tst_brk(TBROK, ...)
all we want is to make the test processes exit as soon as possible.

> > This makes the tst_brk() behavior well defined so we can now even call
> > tst_brk() with TFAIL and TPASS as well.
> 
> What's the use-case for it? Wouldn't it be more clear to just report
> TPASS + exit?

I think this makes actually the API more consistent. I.e. tst_res()
reports result and tst_brk() reports result and exits the current
process.

I think that we all carry a mental baggage that associates the tst_brk()
call with an error, but that is something we forced upon ourselves.
AFAIK it's short for tst_break, which itself only suggests that it does
exit the current process, similar to break being used in switch()
statement.

> > Open question (may be done in a separatep patch):
> >
> > Should tst_brk(TBROK, ...) apart from setting the flag also send sigkill
> > signal to the test process group to kill any leftover test processes?
> 
> Or heartbeat checking the abort flag and doing it from the library?

The heartbeat handler may be a good place to put this check into and I
was also thinking of adding checks to all SAFE_MACROS() and make them
something as cancelation points since they are supposed to exit the test
on a failure anyways.

But that could be only done once we have the flag in place and finally
have persistent way how to check that something went wrong.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

  reply	other threads:[~2024-11-26 14:50 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-15 16:41 [LTP] [PATCH] [RFC] lib/tst_test.c: Fix tst_brk() handling Cyril Hrubis
2024-11-18 10:39 ` Jan Stancek
2024-11-26 14:50   ` Cyril Hrubis [this message]
2024-11-27  9:42     ` Li Wang
2024-12-17 21:46       ` Petr Vorel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Z0XgJVa9czWm2YMc@yuki.lan \
    --to=chrubis@suse.cz \
    --cc=jstancek@redhat.com \
    --cc=ltp@lists.linux.it \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.