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

Properly handle concurrent access to qubes.xml #1729

Closed
marmarek opened this issue Feb 9, 2016 · 4 comments
Closed

Properly handle concurrent access to qubes.xml #1729

marmarek opened this issue Feb 9, 2016 · 4 comments
Assignees
Labels
C: core P: major Priority: major. Between "default" and "critical" in severity.
Milestone

Comments

@marmarek
Copy link
Member

marmarek commented Feb 9, 2016

When two tools concurrently modify qubes.xml, one will "rollback" changes made by the other. Generally because each tool first load qubes.xml, do it's work, then (optionally) rewrite the whole file with new state. In core2 we have two locking methods - lock_db_for_reading and lock_db_for_writing - if one want to modify qubes.xml, he/she needs to take write (exclusive) lock.
Not sure how it should be done here. Maybe somehow similar (but using __init__() or load() parameter)?

@marmarek marmarek added C: core P: major Priority: major. Between "default" and "critical" in severity. task labels Feb 9, 2016
@marmarek marmarek added this to the Release 4.0 milestone Feb 9, 2016
@marmarek
Copy link
Member Author

marmarek commented Feb 9, 2016

As a stopgap I'll add detection of such situation and raise an exception. So at least it will be obvious what happened...

marmarek added a commit to marmarek/old-qubes-core-admin that referenced this issue Feb 10, 2016
For now simply throw an exception. Proper solution require some locking

QubesOS/qubes-issues#1729
woju pushed a commit to woju/qubes-core-admin that referenced this issue Feb 29, 2016
For now simply throw an exception. Proper solution require some locking

QubesOS/qubes-issues#1729
woju pushed a commit to woju/qubes-core-admin that referenced this issue Mar 3, 2016
For now simply throw an exception. Proper solution require some locking

QubesOS/qubes-issues#1729
woju pushed a commit to woju/qubes-core-admin that referenced this issue Mar 3, 2016
For now simply throw an exception. Proper solution require some locking

QubesOS/qubes-issues#1729
@woju
Copy link
Member

woju commented Oct 26, 2016

I'm working on such a schema: .__init__() will have additional argument lock, which when set to True will lock the file (with LOCK_EX) until .save() is executed. If loaded with lock=False, there will be .reload(), which again will have lock argument.

Attempt to .save() without locking first will be as in present situation, best-effort: will either succeed or fail when someone would have overwritten qubes.xml. Alternatively we can try to discourage developers from doing that by unconditionally failing, but it's probably bad idea, since the events would already be fired, so for existing code it would actually be worse than what is now.

There would be no equivalent for .lock_db_for_reading().

@marmarek #1901

@marmarek
Copy link
Member Author

I think this is very good compromise between usability and complexity. 👍

woju added a commit to woju/qubes-core-admin that referenced this issue Oct 27, 2016
woju added a commit to woju/qubes-core-admin that referenced this issue Oct 27, 2016
woju added a commit to woju/qubes-core-admin that referenced this issue Oct 28, 2016
woju added a commit to woju/qubes-core-admin that referenced this issue Oct 28, 2016
@woju
Copy link
Member

woju commented Nov 24, 2016

.reload() proved to be tricky. One can always instantiate new app object, if requred. So closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: core P: major Priority: major. Between "default" and "critical" in severity.
Projects
None yet
Development

No branches or pull requests

2 participants