On 2021/7/20 下午2:43, Dave Chinner wrote:
On Tue, Jul 20, 2021 at 10:45:17AM +0800, Qu Wenruo wrote:
On 2021/7/20 上午10:14, Dave Chinner wrote:
On Tue, Jul 20, 2021 at 08:36:49AM +0800, Qu Wenruo wrote:
On 2021/7/20 上午8:25, Dave Chinner wrote:
On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote:
This patch will allow fstests to run custom hooks before and after each
test case.
These hooks will need to follow requirements:
- Both hook files needs to be executable
Or they will just be ignored
- Stderr and stdout will be redirected to "$seqres.full"
With extra separator to distinguish the hook output with real
test output
Thus if any of the hook is specified, all tests will generate
"$seqres.full" which may increase the disk usage for results.
- Error in hooks script will be ignored completely
- Environment variable "$HOOK_TEMP" will be exported for both hooks
And the variable will be ensured not to change for both hooks.
Thus it's possible to store temporary values between the two hooks,
like pid.
- Start hook has only one parameter passed in
$1 is "$seq" from "check" script. The content will the path of current
test case. E.g "tests/btrfs/001"
- End hook has two parameters passed in
$1 is the same as start hook.
$2 is the return value of the test case.
NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak
check.
For more info, please refer to "README.hooks".
This is all info that should be in README.hooks, not in the commit
message. Commit messages are about explaining why something needs
to exist or be changed, not to describe the change being made. This
commit message doesn't tell me anything about what this is for, so I
can't really make any value judgement on it - exactly what is this
intended to be used for?
To run whatever you may want.
No, don't try to turn this around and put it on me to think up use
cases to justify your change. You have a use case for this, so
*document it so everyone understands what it is*.
If you don't need it, then fine.
But there are already other guys interesting in this feature.
Talk to them too.
Something you don't need doesn't mean other don't.
Hold up, what's with the attitude?
I asked you to describe the use case for the hooks because it wasn't
in the commit message and I don't have a clue how you intend to use
it. Hence I need you, the patch author and submitter, to tell me
what it is intended for.
Shouting at me telling how I should read about what others want when
instead of actually answering my question by describing your use
case as I've asked you (repeatedly) to describe is not helpful.
So please clam down, take a step back and please explain to me in a
calm, rational, professional manner what this functionality is
needed for.
I only need to do the very simple work like call trace-cmd clear and
trace-cmd set -b 4.
But I can see other guys may want such generic hook to do whatever they
want (but still overall simple things).
Bringing a generic simple framework while you can't see the usage to
replace your complex stack isn't the correct way to go.
I admit it's not obvious in the RFC that only simple work should be put
into the hook (aka throw away nature).
My concerns are about whether the infrastructure is maintainable
from a long term persepective, and that all depends on what use
cases we have for it and whether global hooks are the likely best
solution to those use cases over the long term. I'm not opposed to
adding hooks, I'm just asking for context and justification that is
needed to be able to consider if this is the best solution for the
use cases that are put forward...
Nope. All hook users are responsible for whatever they do.
Maybe it's adding trace-cmd calls, maybe it's to do extra error
injection setup, I don't care.
You might not care, but other people will. I can see that if these
execution hooks becomes a common thing people use, it will need to
be formalised and further developed and documented.
This is already your guess.
Again, here the hook is simple, do your work there, if it works, good
the hook works for you.
If it brings some unexpected behavior, you're on your own, RTFC crafted
by yourself.
We will explicitly state that, this feature is only for debug purpose
and use on your risk.
No complain, no code compatibility, if one day code update break your
hook, it's not our problem, it's the one who crafts the hook.
Because if they
are in widespread use, we really, really care about things like API
changes because breaking everyone's custom hook scripts because we
removed or changed a global variable or function is not in anyone's
interests.
You're again building the whole thing on the "if" part.
I just provide a way to do that more simply, to add two points to call
executable scripts, and any modification in their hooks won't be
submitted to fstests by default (check the gitignore update)
What users do in their hooks is not what I really care, and nor what you
should care.
Yes, I get it that you don't care. And that's a problem.
ISTM that you haven't thought this through beyond "add a hook for
running throw-away debug code". You clearly haven't thought about
what a developer would need to do to build a library of hook
implementations for debugging different tests.
No, we are not doing that complex thing whatever you want to utilize.
It's just a simple hook, no guarantee, just to make it simple to do
simple things.
I say that because
maintaining that library via commits in local fstests repositories
is impossible because gitignore rules you added.
Because it's supposed to be simple.
I know, you don't
care, but I very much do, because storing stacks of custom changes
to fstests in local repositories is how I deploy fstests to my test
machines...
If your stack is so complex, this is not for you.
Submit whatever you may want to add to fstests, and try to push a
complex framework for your workload.
And given that it appears you haven't thought about maintaining a
local repository of hooks, I strongly doubt you've even consider the
impact of future changes to the hook API on existing hook scripts
that devs and test engineers have written over months and years
while debugging test failures.
Let me state it again, fstests itself only cares for the test cases.
You use the hook, you're on your own.
Don't expect any stable thing, this is just for simple usage.
Darrick pointed out the difference between running in the check vs
test environment, which is something that is very much an API
consideration - we change the test environment arbitrarily and fix
all the tests that change affects at the same time. But if there are
private scripts that depend on the test environment and variables
being stable, then we can't do things like, say, rename the "seqres"
variable across all tests because that will break every custom hook
script out there that writes to $seqres.full...
See what I'm getting at here? I've looked at the proposal and I'm
asking questions that your implementation raises. I'm not asking
these questions to be difficult, I'm asking them because I want to
know if I can use the hooks to replace some of the things I do with
other methods.
My answer to this question is, this hook is only for simple work.
Either you maintain your complex hook to work for latest fstests, or
just put simple things into it.
If you want more complex work or stable ABI, then this is not for you.
And if I do, how I'm expected to maintain and deploy
them to the test machine farm from my master git repository they all
pull from...
This feature is at least not designed for such usage.
It's more for the through away debug setup.
If someone is running hooks with every test case, it's their problem or
intention. I don't care!
IOWs, without a description of your use case and requirements, I
have no basis from which to determine if this is useful
infrastructure over the long term or not.
OK, my use case is just to run "trace-cmd clear" before one test,
btrfs/160, and I don't want to populate my workplace so that I may
submit some patch with my debugging setup included.
I would only run that btrfs/160 with my custom hook, that's all.
If you have to submit a patch with the debugging and custom hook
defined in it to the build system, why can't you just submit a patch
that, say, runs "trace-cmd clear" at the start of btrfs/160 itself?
IOWs, I don't understand why generic, fstests-global script
execution hooks are needed just to run a simple command for helping
debug a single test. Why not just *patch the test* and then once
you're done throw away the debug patch?
It's again back to the original question.
I only need to run several small commands, but I guess others may be
interested in adding some more complex but still overall simple work.
If it helps you, great.
If it doesn't, just don't use it and go back to what works for you.
Cheers,
Dave.
