Created attachment 229523 [details] [review]
Make MediaManager a singleton

Created attachment 229524 [details] [review]
vm-configuration: Save media & OS info for new VMs

We now put all the custom XML under a toplevel 'gnome-boxes' node. This
breaks compatibility with existing under installation and live boxes out
there but in the sense that Boxes will think that those boxes are already
in installed state. Do we really need to care enough for those cases here
to complicate code and resulting XML?

Created attachment 229525 [details] [review]
installer-media: Mark nullable constructor params as such

Created attachment 229526 [details] [review]
vm-configurator: Add get_source_media_path()

Add a method for retreiving the installer media from domain config.

Created attachment 229528 [details] [review]
media-manager: Add create_installer_media_from_config

Add a method for creation of InstallerMedia from domain config.

Created attachment 229529 [details] [review]
vm-creator: Info on install media now guaranteed

After this patch, VMCreator instance always have access to information
on original installation media even if Boxes is quit during
installation/live session.

Created attachment 229530 [details] [review]
vm-configurator: Refactor custom XML setting code

Refactor custom XML creation and setting into one function.

Created attachment 229531 [details] [review]
vm-creator: Save number of reboots for VM under installation

This means that now we don't lose means to detect completion of
installation just because user quit Boxes before installation was
completed.

Review of attachment 229523 [details] [review]:

ack

Review of attachment 229524 [details] [review]:

::: src/vm-configurator.vala
@@ +30,3 @@
+            custom_xml += OS_ID_XML.printf (install_media.os.id);
+        if (install_media.os_media != null)
+            custom_xml += MEDIA_ID_XML.printf (install_media.os_media.id);

Shouldn't these use Markup.printf_escaped to handle weird chars in the arguments.

@@ +186,3 @@
     private static void mark_as_installed (Domain domain) {
         try {
+            var custom_xml = BOXES_XML.printf (INSTALLED_XML);

This seems to override the other settings?

Review of attachment 229525 [details] [review]:

ack

Review of attachment 229526 [details] [review]:

ack

Review of attachment 229524 [details] [review]:

::: src/vm-configurator.vala
@@ +307,3 @@
                                                         Xml.ParserOption.COMPACT);
             do {
+                if (reader.name () == node_name)

Does this really work right?
Its seems to me like this iterates over all toplevel nodes in the custom xml, which for new data would be a single "gnome-boxes" node.

Also, it wouldn't be too bad to add some backwards compat here. If the toplevel is a "gnome-boxes" child, enumarate children of that. Otherwise enumerate the toplevel node.

Review of attachment 229530 [details] [review]:

::: src/vm-configurator.vala
@@ +342,3 @@
+            domain.set_custom_xml (custom_xml, BOXES_NS, BOXES_NS_URI);
+        } catch (GLib.Error error) { assert_not_reached (); /* We are so screwed if this happens */ }
+    }

I worry somewhat over this. It overwrites *all* custom XML nodes, which is fine as long as you write every node. However, for the future case were we might add a new child it causes us to move said node if you ever run an older version of boxes, which might happen if you share homedir with multiple versions of boxes.

Not sure how much we care about this though, I can't decide...

Review of attachment 229524 [details] [review]:

::: src/vm-configurator.vala
@@ +186,3 @@
     private static void mark_as_installed (Domain domain) {
         try {
+            var custom_xml = BOXES_XML.printf (INSTALLED_XML);

yikes, thats true. Feel a bit lazy to fix it though since its corrected by following changes.

Review of attachment 229530 [details] [review]:

::: src/vm-configurator.vala
@@ +342,3 @@
+            domain.set_custom_xml (custom_xml, BOXES_NS, BOXES_NS_URI);
+        } catch (GLib.Error error) { assert_not_reached (); /* We are so screwed if this happens */ }
+    }

No, set_custom_xml is taking a namespace and namespace_uri for a reason. Also we only set this XML during and at the end of installations, only read after that point.

Review of attachment 229530 [details] [review]:

::: src/vm-configurator.vala
@@ +342,3 @@
+            domain.set_custom_xml (custom_xml, BOXES_NS, BOXES_NS_URI);
+        } catch (GLib.Error error) { assert_not_reached (); /* We are so screwed if this happens */ }
+    }

to be clear, we are only overwriting our own 'gnome-boxes' node here.

Review of attachment 229524 [details] [review]:

::: src/vm-configurator.vala
@@ +307,3 @@
                                                         Xml.ParserOption.COMPACT);
             do {
+                if (reader.name () == node_name)

The custom xml is the gnome-boxes node here, not its parent 'metadata' node in the config. Also, I have indeed tested this and AFAICT, it didn't break anything.

As I mentioned in the log, backwards compatiblity isn't really important here but now that i think about it again, it *is* important for live VMs that people would want to keep around in their "live" state..

Review of attachment 229530 [details] [review]:

::: src/vm-configurator.vala
@@ +342,3 @@
+            domain.set_custom_xml (custom_xml, BOXES_NS, BOXES_NS_URI);
+        } catch (GLib.Error error) { assert_not_reached (); /* We are so screwed if this happens */ }
+    }

I understand that. I'm saying, a later version of boxes may put more inside the gnome-boxes node, and an older version of boxes sharing the home dir, may remove this node if it updates custom_xml.

Review of attachment 229524 [details] [review]:

::: src/vm-configurator.vala
@@ +307,3 @@
                                                         Xml.ParserOption.COMPACT);
             do {
+                if (reader.name () == node_name)

I didn't notice you changed it from iterating shallowly to iterating deeply.
I think this is wrong. You should only look at the toplevel, in case we later add our own nodes that have hierarchical structure.

Review of attachment 229530 [details] [review]:

::: src/vm-configurator.vala
@@ +342,3 @@
+            domain.set_custom_xml (custom_xml, BOXES_NS, BOXES_NS_URI);
+        } catch (GLib.Error error) { assert_not_reached (); /* We are so screwed if this happens */ }
+    }

I think anyone modifying boxes code in future to add more nodes should be looking at the existing custom xml handling code (which are just two simple functions) and should realize that he/she needs to also change how these functions work.

Review of attachment 229524 [details] [review]:

::: src/vm-configurator.vala
@@ +307,3 @@
                                                         Xml.ParserOption.COMPACT);
             do {
+                if (reader.name () == node_name)

ah so we want both backward and forward compatibility. :) I think I know how to achieve that but it will make this code slightly more complicated..

NB: I think it will point to some fundamental problem with libvirt if we start to need a tree inside this custom node.

Created attachment 229731 [details] [review]
vm-configuration: Save media & OS info for new VMs

We now put all the custom XML under a toplevel 'gnome-boxes' node. We do
this without breaking compatibility with existing under installation and
live boxes out there.

Currently this info is only there during the installation but the patch
to make it permanent follows in the same patch series as this.

Created attachment 229733 [details] [review]
vm-configurator: Refactor custom XML setting code

Refactor custom XML creation and setting into one function.

This also makes the OS and media info in the domain config, permanent.

Review of attachment 229530 [details] [review]:

::: src/vm-configurator.vala
@@ +342,3 @@
+            domain.set_custom_xml (custom_xml, BOXES_NS, BOXES_NS_URI);
+        } catch (GLib.Error error) { assert_not_reached (); /* We are so screwed if this happens */ }
+    }

How would you in the future change existing shipped boxes code? We want to make the current code we release supportable in the future if we need to extend the set of custom data.

Review of attachment 229524 [details] [review]:

::: src/vm-configurator.vala
@@ +307,3 @@
                                                         Xml.ParserOption.COMPACT);
             do {
+                if (reader.name () == node_name)

It not necessarily compatible in both directions. We want to properly read data from old toplevel-less nodes, as well as the new ones. We unfortunately can't make older versions handle the new toplevel.

Review of attachment 229731 [details] [review]:

Not sure exactly how this goes in the series, but this looks very right to me.

Review of attachment 229733 [details] [review]:

looks good

Review of attachment 229528 [details] [review]:

ack

Review of attachment 229529 [details] [review]:

ack

Review of attachment 229531 [details] [review]:

ack

I think this looks good to get in.

however, i still worry a bit about how when updating the custom xml we basically rewrite the whole thing. That may very well bite us in the future when we add new things in later versions and they are deleted by older versions running in the same homedir (like say you're sharing a /home partition and boot different distros as / with different boxes versions).

(In reply to comment #32)
> I think this looks good to get in.

Thanks for reviewing.

> however, i still worry a bit about how when updating the custom xml we
> basically rewrite the whole thing. That may very well bite us in the future
> when we add new things in later versions and they are deleted by older versions
> running in the same homedir (like say you're sharing a /home partition and boot
> different distros as / with different boxes versions).

Yeah, I've added in my todo to improve the custom xml writing code.

Created attachment 229912 [details] [review]
vm-creator: Info on install media now guaranteed

This one needed rebasing on top of current master so re-attaching..

Review of attachment 229912 [details] [review]:

ack

Attachment 229523 [details] pushed as ca2c926 - Make MediaManager a singleton
Attachment 229525 [details] pushed as f28db61 - installer-media: Mark nullable constructor params as such
Attachment 229526 [details] pushed as b85cd41 - vm-configurator: Add get_source_media_path()
Attachment 229528 [details] pushed as 57cbb09 - media-manager: Add create_installer_media_from_config
Attachment 229531 [details] pushed as fa1d58d - vm-creator: Save number of reboots for VM under installation
Attachment 229731 [details] pushed as df36a76 - vm-configuration: Save media & OS info for new VMs
Attachment 229733 [details] pushed as db72eb4 - vm-configurator: Refactor custom XML setting code
Attachment 229912 [details] pushed as 6c112d3 - vm-creator: Info on install media now guaranteed