dinit-service: The new `restart = on-failure` option!

[mobin-2008]

Commits are self-explanatory. It also has a build-time configuration for setting it.

Fixes #271

Signed-off-by: Mobin Aydinfar <mobin@mobintestserver.ir>

[mobin-2008]

@lfxx Can you test my branch? I tested it and everything works as expected, Just to make sure, compile it and run a daemon with restart = on-failure option.

[davmac314]

@mobin-2008 review was requested but you are still pushing changes. What's the deal? Is it ready for review or not?

[mobin-2008]

I just added a small comments in configs about those auto-restart conflict, No more change. Sorry about confusion, Everything is ready for review.

[mobin-2008]

• The change in src/tests/test-includes/baseproc-sys.h doesn't make sense to me,. won't this just always return 0?

https://github.com/davmac314/dinit/blob/213d3fb1bff820f9cbe981193ad06c103d787405/src/includes/proc-service.h#L536-L539

As you see "base_process_service" exit_status is checked through as_int() and I guess it represents the exit code of process. I tested my patches with return 0 and fail-only restart test failed:

...
test_proc_term_restart5...    proctests: proctests.cc:492: void test_proc_term_restart5(): Assertion `p.get_target_state() == service_state_t::STARTED' failed.
...

So I think we should return status in that header.

[davmac314]

I see I was wrong, it doesn't mean that it always returns 0. But returning status is also wrong. It should instead return a value (v) so that WEXITSTATUS(v) is equal to status. There's no portable way to do that currently.

I have made a note that exit status handling needs some rework, so if necessary you can leave the test disabled for now.

[mobin-2008]

I don't understand the logic of setting current exit_status. In production code caller should set exit_status of service from POSIX status code. But we also have a function called handle_exit_status() which also gets a exit_status in its parameters.

So what did you mean by doing both of them? If handle_exit_status() should be responsible for setting current exit_status, Why it doesn't? or if it's not responsible for doing so, it should use current exit_status from service variables, not getting it from its parameters.

https://github.com/davmac314/dinit/blob/15c61b59b74f7eab40718a91ee78e2ada8dc5d12/src/proc-service.cc#L242-L258 According to this, I choose first approach, Caller should set exit_status before calling handle_exit_status() and handle_exit_status() should use it from service variables.

[mobin-2008]

Please do not rebase in-progress PRs onto master if it is not actually necessary. When it has been rebased I cannot compare it to the previous version of the PR and this makes it much harder to review.

I didn't know that, I will rebase in-progress PRs only if there is conflict.

@davmac314 I squashed commits in pr-338 branch and it's ready for review.

[davmac314]

I squashed commits in pr-338 branch and it's ready for review.

You need to push the changes to your own branch for them to show up in the PR.

[mobin-2008]

I squashed commits in pr-338 branch and it's ready for review.

You need to push the changes to your own branch for them to show up in the PR.

Can you merge it manually from that branch?

[davmac314]

Can you merge it manually from that branch?

I would like to look over it beforehand, can you please push the changes to your branch

[mobin-2008]

I would like to look over it beforehand, can you please push the changes to your branch

I think it will be better to open a new PR for that purpose.

[davmac314]

@mobin-2008 No, just push the commits to your PR branch please. I don't want to lose the review history either.

This should be very easy to do.

[mobin-2008]

@mobin-2008 No, just push the commits to your PR branch please. I don't want to lose the review history either.

This should be very easy to do.

That's OK, I'm going to do that.