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

Initial autoupdate implementation #153

Merged
merged 9 commits into from
Aug 8, 2017
Merged

Initial autoupdate implementation #153

merged 9 commits into from
Aug 8, 2017

Conversation

main--
Copy link
Owner

@main-- main-- commented Aug 8, 2017

Needs more testing.

Fixes #126.

@main-- main-- added this to the Beta release milestone Aug 8, 2017
@main-- main-- self-assigned this Aug 8, 2017
@main-- main-- requested review from oberien and moritzuehling August 8, 2017 11:35
@oberien
Copy link
Collaborator

oberien commented Aug 8, 2017

Is it possible to attach the cd drive dynamically with device_add only if the GA is outdated, and remove it again when the GA is updated? I think it feels weird to have the GA.iso attached at all times.

Otherwise lgtm

@main--
Copy link
Owner Author

main-- commented Aug 8, 2017

@oberien That's what I was planning to do initially. However, there's no straightforward way to tell if/when the device is going to be ready. Plus, implementing the required QMP commands is somewhat weird (see #131).

Permanently attaching the GA ISO is definitely not perfect but not that big of a deal either. It's there, whatever, just ignore it.

Hypervisors have traditionally addressed this by creating an optical drive at boot time and allowing you to insert/remove ISO images at runtime. This does not work here however as the corresponding qemu APIs are still unstable/experimental.

@oberien
Copy link
Collaborator

oberien commented Aug 8, 2017

You should create a new issue then just to keep it in mind. (Also don't forget to update the reference in #131 ).

If it wasn't for "Needs more testing.", I'd approve this PR.

Copy link
Collaborator

@moritzuehling moritzuehling left a comment

Choose a reason for hiding this comment

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

Gernerally I'd probably prefer one of the two methods we discussed.

let latest = Version::parse(include_str!("../../../guest-agent/VfioService/VfioService/Resources/Version.txt")).unwrap();
match Version::parse(&version) {
Ok(version) => {
if version < latest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

!= instead of <

@@ -46,6 +46,9 @@ pub fn run(cfg: &Config, tmp: &Path, data: &Path, clientpipe_path: &Path, monito
debug!("Samba started");
}

let ga = data.join("windows-gaming-ga.iso");
Copy link
Collaborator

Choose a reason for hiding this comment

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

naming: gaIso or something like that to easier see what this is. ga can have multiple meanings

oberien
oberien previously requested changes Aug 8, 2017
Copy link
Collaborator

@oberien oberien left a comment

Choose a reason for hiding this comment

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

@main-- main-- merged commit a4693b9 into master Aug 8, 2017
@oberien oberien deleted the euro6 branch August 8, 2017 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants