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

Already on GitHub? Sign in to your account

Bug7101 #1

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
Member

oldpatricka commented Sep 13, 2010

This branch is to fix a problem discussed starting here in Campfire, and explained further in bug 7101.

To solve this, I replaced the current locking solution with flock, as in the other libexec scripts.

I'm fairly confident about this part of the patch, but I noticed there's some locking in dhcp-conf-alter.py. I don't think there should be any side effects, but I don't know. Hence, the pull request.

oldpatricka and others added some commits Sep 13, 2010

Replace home-made locking solution with flock in dhcp-config.sh
This is just like in the ebtables-config and mount-alter scripts. This was done to address bug 7101, which can cause problems when workspace-control is installed on a shared filesystem.
Owner

priteau commented Sep 14, 2010

Since dhcp-config.sh protects the whole block of code containing dhcpd shutdown, configuration file altering and dhcpd restart, dhcp-conf-alter.py should be able to run without any sort of locking.

Member

labisso commented Sep 14, 2010

Has this been tested? The locking within dhcp-conf-alter.py seems redundant from what I can tell.

Member

oldpatricka commented Sep 14, 2010

I tested it on one of our systems, and seems to work well.

Remove the useless locking from dhcp-conf-alter.py
The locking is being done by dhcp-config.sh which itself calls
dhcp-conf-alter.py.
Owner

priteau commented Sep 14, 2010

I will try testing a diff with the locking removed from dhcp-conf-alter.py.

Member

oldpatricka commented Sep 14, 2010

Hmm, also, does the installer need to touch the new dhcp lock file? Does that happen with the ebtables lock file?

Owner

priteau commented Sep 15, 2010

Yes, the dhcp lock file would need to exist, because you used 200<$FLOCKFILE.
This doesn't happen with the ebtables scripts because they use > or >>.

However, I discovered that there are many more issues with this patch (I will push fixes soon):

  • If an error happens in the block of code protected by flock, it is not caught by set -e, because the parentheses create a subshell. The solution is to do something like ( flock ... ) 200>$FLOCKFILE || exit $?, which will exit if the status code returned by the subshell is non-zero.
  • On my system (Debian Lenny), the DHCP daemon which is started in the subshell inherits the lock. Since it is daemonized, the lock file stays opened and prevents any subsequent dhcp-config.sh execution. I tried every option in flock but I didn't find any way to close an existing file description and execute a command (-o opens a new file descriptor and then closes it). But there is a solution, close the file ourselves with a bash command: exec 200>&-
Fix dhcp-config.sh locking
* Use >> instead of < because the lock file might not exist.
* Add || exit $? after the subshell, otherwise any error will not be
  caught.
* Close the file descriptor before starting dhcpd, otherwise the
  daemon will keep the lock indefinitely.
Member

labisso commented Sep 15, 2010

As Pierre noted in campfire, this is likely the reason that dhcp-config.sh used the alternate locking mechanism instead of flock. Ring any bells, Tim?

Member

oldpatricka commented Sep 15, 2010

Ahh, cool. Thanks Pierre. It seemed to work on Red Hat, so I'm glad you were able to test it on Debian.

Owner

priteau commented Sep 15, 2010

Maybe some code in Red Hat closes file descriptors before starting the DHCP server, that could explain why it worked. And I assumed you had created the /var/lock/dhcp-config.lock file beforehand?

So, could you give this new patchset a spin? I think we could have a final version here.

Owner

priteau commented Oct 7, 2010

Pushed to master after a rebase (no change), thank you everyone.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment