Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use 'set -o errexit' to catch script errors #89

Open
hreinecke opened this issue Apr 19, 2022 · 6 comments
Open

Use 'set -o errexit' to catch script errors #89

hreinecke opened this issue Apr 19, 2022 · 6 comments

Comments

@hreinecke
Copy link
Contributor

Heya,

bash has the option 'errexit' (invoked via 'set -o errexit'), which will cause the script to abort if a command/line returns 'false'.
This has the nice effect that any errors are catched automatically, and don't have to be checked for.
With that we'll be able to catch silent failures which are currently ignored.
Drawback is that one gets lots of false positive initially, as every command which should be ignored need to be suffixed by ' || true'.

But it's a very good thing for hardening the scripts; I've had quite good experience with that when developing testcases for md_monitor

@kawasaki
Copy link
Collaborator

kawasaki commented Sep 5, 2023

As I noted at LSF 2023, I think this is a good idea. Yesterday, I found time to work on this, and did a trial implementation: a99d840. With this change, I observed many test cases fail or show weird behavior as @hreinecke indicated. It looks like certain amount of work is required to fix them all. I will spend some more time and see how much work will be required.

@bvanassche
Copy link
Contributor

Is set -o errexit equivalent to set -e? Do others have a preference for one of these two alternatives? I slightly prefer set -e. set -e already occurs in the file new.

@kawasaki
Copy link
Collaborator

Is set -o errexit equivalent to set -e? Do others have a preference for one of these two alternatives? I slightly prefer set -e. set -e already occurs in the file new.

Thanks for the comment. I will user set -e.

@kawasaki
Copy link
Collaborator

I've spent some time on the errexit support. The number of affected test cases are not so small, but it looks possible to modify them to meet the errexit requirements. Around a dozen of patches will be required. WIP code is available at my errexit branch.

Through this work, I noticed following points:

  1. Some commands in test cases need to return non-zero error status to meet the test purpose. For those commands, we need to add "|| true". This looks a bit weird and I'm not sure if blktests contributors will like it or not. Some discussion on the list will be needed.
  2. If status check of commands connected with pipes, some complicated changes will be required. scsi/006 is the test case with such a pipe. This adds some complexity, but I think such cases are rare and should be fine.
  3. With current implementation, some of clean up operations after each test can be skipped when errexit happens in the test case. This is not good. It is desired to improve clean up in the blktests framework before errexit introduction.
  4. I found that some of the commands in nbd/002 and nbd/003 return non-zero status, and not sure if these are expected or not. Will need further work.

In short, this errexit support work needs larger effort than I expected. I will rethink priority of this work. I think nbd/002,003 improvement and clean-up improvement will have higher priority.

@bvanassche
Copy link
Contributor

How about using this alternative for ignoring an error status:

if command_that_may_fail; then :; fi

@kawasaki
Copy link
Collaborator

kawasaki commented Sep 26, 2023

How about using this alternative for ignoring an error status:

if command_that_may_fail; then :; fi

That will work too, and could be easier to understand for some people :) One drawback is that this needs more types than "|| true".

This made me think of creating a new helper function like _ok() or _ignore_status(). With these it will be "|| _ok" or "|| _ignore_status", and these might be a bit easier to understand than "|| true".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants