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

Fix #978: mark started flag as false after failed start on reload. #979

Merged
merged 2 commits into from
Mar 26, 2018

Conversation

aleksostapenko
Copy link
Contributor

No description provided.

Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general the fix looks good. Probably, the one place can be improved. Review from @ikoveshnikov is required since he wrote the reconfiguration procesing which the patch affects.

@@ -269,8 +271,7 @@ tfw_ctlfn_state_change(const char *old_state, const char *new_state)
TFW_LOG("Live reconfiguration of Tempesta.\n");
}

if (!(r = tfw_start(&tfw_mods)))
WRITE_ONCE(tfw_started, true);
r = tfw_start(&tfw_mods);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current tfw_ctlfn_state_change() is good that all the modification of tfw_started are there and the patch distributes the variable updates over the code. Why not just to change these 2 line like?

r = tfw_start(&tfw_mods);
WRITE_ONCE(tfw_started, !r);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of true this variant is ok. but in case of false - it disables tfw_started flag in both ( reconfiguration failed and start failed) events, and we need to disable it only in event of failed start. In failed reconfiguration case Tempesta remain in started state, so we need to preserve tfw_started flag in true value.

Copy link
Contributor

@vankoven vankoven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to merge!

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

Successfully merging this pull request may close these issues.

3 participants