Created attachment 205580 [details] [review]
installer: Domain & its volume should share name

If simply create the domain and its main storage volume by the same
name, its much faster/easier to find the volume given a domain.

Created attachment 205581 [details] [review]
installer: More relible way to track installations

* Don't create transient domains for live case but rather delete the
  domain (and its volume) ourselves. The issue with transient domains is
  that once they are shutdown, there is no way to get their configuration
  to base a new permanent configuration to base on.
* Make use of 'description' node in domain configuration to keep track
  of an installation/live session in progress. Without this change,
  Boxes is not able to do the necessary post-installation setup if user
  quits Boxes during installation in progress.

Created attachment 205582 [details] [review]
installer: More relible way to track installations

* Don't create transient domains for live case but rather delete the
  domain (and its volume) ourselves. The issue with transient domains is
  that once they are shutdown, there is no way to get their configuration
  to base a new permanent configuration to base on.
* Make use of 'description' node in domain configuration to keep track
  of an installation/live session in progress. Without this change,
  Boxes is not able to do the necessary post-installation setup if user
  quits Boxes during installation in progress.

Review of attachment 205581 [details] [review]:

perhaps the post-install setup should be handled by libvirt, I don't know if there is a way to do an "exec-after-reboot" or something similar. That would be more reliable.

::: src/vm-configurator.vala
@@ +72,3 @@
 
+    public bool is_install_config (Domain domain) {
+        return domain.description != null && domain.description.has_suffix (INSTALL_SUFFIX);

It would be better to have app-specific elements in the XML. I think this is possible, just missing some libvirt-glib helper.

(In reply to comment #4)
> Review of attachment 205581 [details] [review]:
> 
> perhaps the post-install setup should be handled by libvirt, I don't know if
> there is a way to do an "exec-after-reboot" or something similar. That would be
> more reliable.

That would be better indeed in the sense that the more (generic) functionality into the stack, the better but I don't think that will improve reliability *much*. Anyway, we can think about that.

> ::: src/vm-configurator.vala
> @@ +72,3 @@
> 
> +    public bool is_install_config (Domain domain) {
> +        return domain.description != null && domain.description.has_suffix
> (INSTALL_SUFFIX);
> 
> It would be better to have app-specific elements in the XML. I think this is
> possible, just missing some libvirt-glib helper.

Yeah, if only libvirt-gconfig provided xmlNode for every object just like gupnp-av. :)

Review of attachment 205580 [details] [review]:

::: src/util.vala
@@ +280,3 @@
     }
 
+    public GVir.StorageVol? get_storage_volume (GVir.Connection connection, GVir.Domain domain) {

I suppose you intend to move this function in libvirt-glib? if not then why moving it to util?

ack otherwise

(In reply to comment #5)
> (In reply to comment #4)
> > ::: src/vm-configurator.vala
> > @@ +72,3 @@
> > 
> > +    public bool is_install_config (Domain domain) {
> > +        return domain.description != null && domain.description.has_suffix
> > (INSTALL_SUFFIX);
> > 
> > It would be better to have app-specific elements in the XML. I think this is
> > possible, just missing some libvirt-glib helper.
> 
> Yeah, if only libvirt-gconfig provided xmlNode for every object just like
> gupnp-av. :)

The thing is that if you look at the strings I'm putting and removing from description for these predicates to work, they are really not misusing this property in any way. i-e If another UI choose to show these strings from libvirt to the user, these strings actually say something. If you like, I can put something even more user-friendly for that situation as well.

(In reply to comment #6)
> Review of attachment 205580 [details] [review]:
> 
> ::: src/util.vala
> @@ +280,3 @@
>      }
> 
> +    public GVir.StorageVol? get_storage_volume (GVir.Connection connection,
> GVir.Domain domain) {
> 
> I suppose you intend to move this function in libvirt-glib? if not then why
> moving it to util?

No I don't. I moved it there because I'm using it in VMCreator as well in the next patch and since I couldn't decide where to put this so I put it in Util since its a generic-enough function. Suggestions welcome!

Created attachment 205731 [details] [review]
installer: More relible way to track installations

* Don't create transient domains for live case but rather delete the
  domain (and its volume) ourselves. The issue with transient domains is
  that once they are shutdown, there is no way to get their configuration
  to base a new permanent configuration to base on.
* Make use of 'description' node in domain configuration to keep track
  of an installation/live session in progress. Without this change,
  Boxes is not able to do the necessary post-installation setup if user
  quits Boxes during installation in progress.

Review of attachment 205731 [details] [review]:

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

what happen if the VM is created by virt-manager or manually? are we handling this case correcty?

(In reply to comment #10)
> Review of attachment 205731 [details] [review]:
> 
> ::: src/vm-configurator.vala
> @@ +77,3 @@
> +        try {
> +            domain.set_custom_xml (INSTALLED_XML, BOXES_NS, BOXES_NS_URI);
> +        } catch (GLib.Error error) { assert_not_reached (); /* We are so
> screwed if this happens */ }
> 
> what happen if the VM is created by virt-manager or manually? are we handling
> this case correcty?

This call is setting the node and the only reason an error will occur here is that *we* are providing incorrect XML.

However later we are "handling" the error in the same way when trying to get this XML but even there I don't see any good chances of getting an error since libvirt errors out if incorrect xml is given to it in the first place and the main reason for get_custom_xml() to throw error is to be complete.

obviously, my comment was meant to be on the get, for existing or externally created VMs. although I didn't test the patch because it depends on other stuff atm, it looks to me like it doesn't handle this case very well and will simply ignore or abort on them. This is not really nice. Boxes should be compatible with any libvirt machine. Could you just make see if it keeps this ability?

(In reply to comment #12)
> obviously, my comment was meant to be on the get, for existing or externally
> created VMs. although I didn't test the patch because it depends on other stuff
> atm, it looks to me like it doesn't handle this case very well and will simply
> ignore or abort on them. This is not really nice. Boxes should be compatible
> with any libvirt machine. Could you just make see if it keeps this ability?

Yeah, I think I attached this here a bit too early. I really should emit a warning in case of getters and return appropriate default value.

Created attachment 205937 [details] [review]
installer: More relible way to track installations

* Don't create transient domains for live case but rather delete the
  domain (and its volume) ourselves. The issue with transient domains is
  that once they are shutdown, there is no way to get their configuration
  to base a new permanent configuration to base on.
* Make use of custom XML in domain configuration to keep track of an
  installation/live session in progress. Without this change, Boxes is not
  able to do the necessary post-installation setup if user quits Boxes
  during installation in progress.

Created attachment 206825 [details] [review]
installer: More relible way to track installations

* Don't create transient domains for live case but rather delete the
  domain (and its volume) ourselves. The issue with transient domains is
  that once they are shutdown, there is no way to get their configuration
  to base a new permanent configuration to base on.
* Make use of custom XML in domain configuration to keep track of an
  installation/live session in progress. Without this change, Boxes is not
  able to do the necessary post-installation setup if user quits Boxes
  during installation in progress.

Review of attachment 206825 [details] [review]:

looks good to me, but I lack of time to test it now.. if you are 100% sure, go for it.

Review of attachment 206825 [details] [review]:

s/relible/reliable btw

Created attachment 207569 [details] [review]
installer: Domain & its volume should share name

If simply create the domain and its main storage volume by the same
name, its much faster/easier to find the volume given a domain.

Created attachment 207570 [details] [review]
installer: More reliable way to track installations

* Don't create transient domains for live case but rather delete the
  domain (and its volume) ourselves. The issue with transient domains is
  that once they are shutdown, there is no way to get their configuration
  to base a new permanent configuration to base on.
* Make use of custom XML in domain configuration to keep track of an
  installation/live session in progress. Without this change, Boxes is not
  able to do the necessary post-installation setup if user quits Boxes
  during installation in progress.

(In reply to comment #16)
> Review of attachment 206825 [details] [review]:
> 
> looks good to me, but I lack of time to test it now.. if you are 100% sure, go
> for it.

I did test the patches and they worked and actually helped. However I had to modify a few bits during rebase so I'll do a bit of testing again before I pushed the rebased patches (already attached).

Created attachment 207585 [details] [review]
installer: More reliable way to track installations

* Don't create transient domains for live case but rather delete the
  domain (and its volume) ourselves. The issue with transient domains is
  that once they are shutdown, there is no way to get their configuration
  to base a new permanent configuration to base on.
* Make use of custom XML in domain configuration to keep track of an
  installation/live session in progress. Without this change, Boxes is not
  able to do the necessary post-installation setup if user quits Boxes
  during installation in progress.

(In reply to comment #21)
> Created an attachment (id=207585) [details] [review]
> installer: More reliable way to track installations

After some testing I found an issue with detection of live session and realized that we better actually parse the xml and match the node value exactly for this to work reliably. This patch does that.

ping? are the rebased and corrected patches still ok?

ack both

Attachment 207569 [details] pushed as 38a1874 - installer: Domain & its volume should share name
Attachment 207585 [details] pushed as be2d57c - installer: More reliable way to track installations