Skip to content
This repository has been archived by the owner on Sep 26, 2021. It is now read-only.

FIX #2154 query virtualbox serially #2172

Merged

Conversation

dgageot
Copy link
Member

@dgageot dgageot commented Nov 4, 2015

Signed-off-by: David Gageot david@gageot.net

@nathanleclaire
Copy link
Contributor

Instead of complicating this code further and adding more provider-specific stuff in, we should just offload responsibility to the driver by throwing a sync.Mutex around calls to VBoxManage (have a PR for this in the wings). Otherwise folks (including us) will be more tempted write unsafe code in their drivers and expect the core to work around it.

I think a mutex around VBoxManage might let us take out the serial bit we have for virtualbox in our run-command-for-context bit as well, but we'll do one step at a time.

@dgageot
Copy link
Member Author

dgageot commented Nov 5, 2015

Hey @nathanleclaire, closing this PR was a bit brutal. No offense taken though since I do really prefer your solution. However, since #2179 is not currently fixing this issue, I'm going to reopen this PR. Please consider it as a working quick and a bit dirty fix that gives you plenty of time to find a better fix.

@dgageot dgageot reopened this Nov 5, 2015
@dgageot dgageot force-pushed the 2154-query-virtualbox-serially branch from 88399f4 to 63371a0 Compare November 5, 2015 11:26
@dgageot
Copy link
Member Author

dgageot commented Nov 5, 2015

@nathanleclaire Please take a look at this updated PR.

@nathanleclaire
Copy link
Contributor

This solution seems pretty reasonable. I have a few nits around style that I am thinking about making a PR to your repo for. Stay tuned.

@nathanleclaire
Copy link
Contributor

I'm not sure that this persists RawDriver correctly on create. Can you confirm that it works for you?

@nathanleclaire
Copy link
Contributor

dgageot#1

Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>
@dgageot dgageot force-pushed the 2154-query-virtualbox-serially branch from 35389b8 to 68092b3 Compare November 7, 2015 15:14
@dgageot
Copy link
Member Author

dgageot commented Nov 7, 2015

@nathanleclaire Thanks for the improvements. PTAL

@nathanleclaire
Copy link
Contributor

LGTM

nathanleclaire added a commit that referenced this pull request Nov 9, 2015
@nathanleclaire nathanleclaire merged commit bc7da2b into docker:master Nov 9, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants