GNOME Bugzilla – Bug 754500
Don't show import option for unimportable VMs
Last modified: 2016-09-20 08:16:03 UTC
After trying some virtual machines with virt-manager, I dedicated myself to delete them, and uninstall the application. When you start gnome-boxes and create a new box on the source selector, multiple entries of the type: "Import << name >> from an agent system". These seem to be the virtual machines created with virt-manager, however, are not operational because they were eliminated. Gnome-boxes should be able to verify the integrity of the inputs or after attempting to import one and not find the disc image, remove the input from source selector. Hugo
I'm guessing you assume deletion of VMs mean simply to delete the disk images but that is not the case. The VM configuration is still there so I'd suggest you delete them using virt-manager or virsh. As for Boxes not showing the option to import VMs without disk images, I'd be more inclined to make it possible to import such VMs (cause it's a valid use case). There is an open bug about it. Having said that, we probably shouldn't provide this option for VMs that have a invalid disk image in their configuration.
(In reply to Zeeshan Ali (Khattak) from comment #1) > I'm guessing you assume deletion of VMs mean simply to delete the disk > images but that is not the case. The VM configuration is still there so I'd > suggest you delete them using virt-manager or virsh. > I understand that it can happen that, however, I deleted my data from my virtual machines with virt-manager (GUI). But it seems that the configuration data remain. Now I will review from the CLI or read the documentation to use virsh. Cheers
(In reply to haevalencia from comment #2) > (In reply to Zeeshan Ali (Khattak) from comment #1) > > I'm guessing you assume deletion of VMs mean simply to delete the disk > > images but that is not the case. The VM configuration is still there so I'd > > suggest you delete them using virt-manager or virsh. > > > > I understand that it can happen that, however, I deleted my data from my > virtual machines with virt-manager (GUI). But it seems that the > configuration data remain. If you deleted the VM from virt-manager, it's a bug in virt-manager if VM configuration is still around. > Now I will review from the CLI or read the documentation to use virsh. # To find the name of VMs for VM in `virsh -c qemu:///system list --all|tail -n +3|head -n -1|cut -d' ' -f7`; do virsh undefine $VM; done
(In reply to Zeeshan Ali (Khattak) from comment #3) > (In reply to haevalencia from comment #2) > > (In reply to Zeeshan Ali (Khattak) from comment #1) > > > I'm guessing you assume deletion of VMs mean simply to delete the disk > > > images but that is not the case. The VM configuration is still there so I'd > > > suggest you delete them using virt-manager or virsh. > > > > > > > I understand that it can happen that, however, I deleted my data from my > > virtual machines with virt-manager (GUI). But it seems that the > > configuration data remain. > > If you deleted the VM from virt-manager, it's a bug in virt-manager if VM > configuration is still around. > > > Now I will review from the CLI or read the documentation to use virsh. > > # To find the name of VMs Disregard that comment. :) > for VM in `virsh -c qemu:///system list --all|tail -n +3|head -n -1|cut -d' > ' -f7`; do virsh undefine $VM; done
(In reply to Zeeshan Ali (Khattak) from comment #3) > > If you deleted the VM from virt-manager, it's a bug in virt-manager if VM > configuration is still around. > I will do a bug report to virt-manager. > > Now I will review from the CLI or read the documentation to use virsh. > > # To find the name of VMs > for VM in `virsh -c qemu:///system list --all|tail -n +3|head -n -1|cut -d' > ' -f7`; do virsh undefine $VM; done By applying the command, the output is: <VM name>: the order was not found... By using the "virsh list" command, no VM listed. Is there a way to remove the configuration data? Thank you for your help. Hugo
(In reply to haevalencia from comment #5) > (In reply to Zeeshan Ali (Khattak) from comment #3) > > By using the "virsh list" command, no VM listed. > > Is there a way to remove the configuration data? > > Thank you for your help. > > Hugo I managed to remove the configuration data with the command "virsh undefine <VM name>". Hugo
Considering that the issue was in fact caused by virt-manager not properly deleting the VM configuration and that importing VMs without a disk image is a valid use case, shouldn't this bug be marked as closed?
(In reply to Visarion Alexandru from comment #7) > Considering that the issue was in fact caused by virt-manager not properly > deleting the VM configuration and that importing VMs without a disk image is > a valid use case, shouldn't this bug be marked as closed? Well it's a pretty easy mistake to make and this bug is not about allowing to import such VMs but rather to ignore such non-functional VMs.
Created attachment 324579 [details] [review] Ignore importing VMs with no virtual disk image
Review of attachment 324579 [details] [review]: As I said to another student, normally I'd just correct these small issues myself while merging but since you intend to work on Boxes full time for a few months, I'll be asking you to fix everything so that you can learn all this early on. :) Shortlog lacks a prefix and "Ignore unimportable VMs" would be a better log. Details assume reader to know about the exact details of the code being modified (what fetched list?). Also (especially since you didn't prefix the shortlog) it's not clear what VMs you are talking about. I'll review the actual code once you have fixed the formatting/style. ::: src/libvirt-system-importer.vala @@ +14,3 @@ owned get { var num_domains = domains.length (); + irrelevant change, please remove. @@ +43,3 @@ domains = system_virt_connection.get_domains (); + + var domains_todel=new GLib.List<GVir.Domain>(); * 'to_del' would be more readable. * lack of spaces * around = * before () @@ +45,3 @@ + var domains_todel=new GLib.List<GVir.Domain>(); + + foreach(var domain in domains){ lack of spaces again. @@ +46,3 @@ + + foreach(var domain in domains){ + GVirConfig.Domain config; extre indented. @@ +61,3 @@ + debug ("Could not find valid disk image for %s",domain_todel.get_name()); + domains.remove(domain_todel); + } * indentation is all over the place here. It's like that your tab is inserting tabs and spaces. Please configure it to use spaces only. * lack of spaces all over. Please read the CodingStyle docs and read through rest of the code and keep new code consistent with existing code. @@ +131,3 @@ if (disk.get_guest_device_type () == GVirConfig.DomainDiskGuestDeviceType.DISK) { disk_path = disk.get_source (); + another redundant change.
Created attachment 324627 [details] [review] Ignore unimportable VMs *fixed coding style issues.
Review of attachment 324627 [details] [review]: Getting there! * Space after ':' in shortlog please. * I'd like you to give another try to describing the patch without getting into details of the code/implementation (HINT: you don't even have to mention existance of any list). Just try your best to describe the change in English. :) Reading through log messages of previous commits in the project might be educational (especially the ones with a bugzilla URL and not from me). Don't worry if it comes shorter description, some patches don't need much explanation. ::: src/libvirt-system-importer.vala @@ +44,3 @@ domains = system_virt_connection.get_domains (); + var domains_to_del = new GLib.List<GVir.Domain> (); + foreach(var domain in domains){ You really gotta get used to having more spaces in code to comply with our coding style. :) @@ +46,3 @@ + foreach(var domain in domains){ + GVirConfig.Domain config; + string disk_path; * Unlike in C, in vala you don't need to forward declare local variables. Just use var and declare them when you initialize them and save lines. * If you need to declare variables, always do so in the block they are needed in. In this case, it would be the 'try' block below. @@ +50,3 @@ + get_domain_info (domain, out config, out disk_path); + var file = File.new_for_path(disk_path); + if(!file.query_exists()) I'd rather you also ensure it's readable. We actually already have a method right in this class for that: ensure_disks_readable(). @@ +53,3 @@ + domains_to_del.append(domain); + } catch (GLib.Error error) { + warning ("%s", error.message); In case of error, we don't want to import the VM. It's likely there is some issue with disk image. @@ +57,3 @@ + } + foreach(var to_del in domains_to_del) { + debug ("Could not find valid disk image for %s",to_del.get_name()); last ( lacks a space before it. @@ +58,3 @@ + foreach(var to_del in domains_to_del) { + debug ("Could not find valid disk image for %s",to_del.get_name()); + domains.remove(to_del); spaces again. @@ +59,3 @@ + debug ("Could not find valid disk image for %s",to_del.get_name()); + domains.remove(to_del); + } Instead of two loops, you can just do it through one loop by constructing the list of domains to use rather than to remove.
Review of attachment 324627 [details] [review]: ::: src/libvirt-system-importer.vala @@ +50,3 @@ + get_domain_info (domain, out config, out disk_path); + var file = File.new_for_path(disk_path); + if(!file.query_exists()) and since you'll be moving the check earlier, the existing check (at the time of import) becomes redundant so you want to remove that in this patch. This also changes a bit what this patch is about: Ignore unimportable VMs early.
Created attachment 324773 [details] [review] Ignore unimportable VMs early on Patch 1/2
Created attachment 324775 [details] [review] Remove old readability check Patch 2/2
Review of attachment 324773 [details] [review]: Moving implies removal so you can squash the second patch into this one (i-e in this case, one patch is more logical). Kudos on good commit log! ::: src/libvirt-system-importer.vala @@ +43,3 @@ + var domains_temp = system_virt_connection.get_domains (); + string[] disk_paths={}; * spaces missing again. * Since you are going to have to create the disk_paths list here anyway, you should keep the disk_paths in the object itself so that import() doesn't have to redo this all over again.
Created attachment 325069 [details] [review] Ignore unimportable VMs early on
Created attachment 325071 [details] [review] *small log change
Review of attachment 325071 [details] [review]: Keep the short log of the patch as the title of the patch, just add your comment below the first line when uploading using `git-bz attach -e`. ::: src/libvirt-system-importer.vala @@ +10,3 @@ private GVir.Connection connection; private GLib.List<GVir.Domain> domains; + private string[] disk_paths; Something went wrong with uploading or rebase cause this is the only change I see.
Review of attachment 325071 [details] [review]: ::: src/libvirt-system-importer.vala @@ +10,3 @@ private GVir.Connection connection; private GLib.List<GVir.Domain> domains; + private string[] disk_paths; I can see the changes here: https://bugzilla.gnome.org/review?bug=754500&attachment=325071 so i'm sure it's not your fault. I'll contact the bz admins..
To avoid having to wait on them (they seem unavailable right now), I'll review here by copy&pasting: > @@ -, +, @@ > src/libvirt-system-importer.vala | 41 ++++++++++++++++++++++++++++++---------- > 1 file changed, 31 insertions(+), 10 deletions(-) > --- a/src/libvirt-system-importer.vala > +++ a/src/libvirt-system-importer.vala > @@ -9,6 +9,7 @@ > private class Boxes.LibvirtSystemImporter: GLib.Object { > private GVir.Connection connection; > private GLib.List<GVir.Domain> domains; > + private string[] disk_paths; > public string wizard_menu_label { > owned get { > @@ -41,7 +42,36 @@ > public async LibvirtSystemImporter () throws GLib.Error { > connection = yield get_system_virt_connection (); > - domains = system_virt_connection.get_domains (); > + var domains_temp = system_virt_connection.get_domains (); > + domains = new GLib.List<GVir.Domain> (); > + disk_paths = {}; With this disk_path in local scope, the disk_paths you added to class never gets used. I don't think the actual import will import anything. Same thing about domains. > + > + foreach (var domain in domains_temp) { > + try { > + string disk_path; > + var config = new GVirConfig.Domain (); > + > + get_domain_info (domain, out config, out disk_path); > + > + var file = File.new_for_path(disk_path); > + if(file.query_exists ()){ > + domains.append(domain); > + disk_paths+=disk_path; > + } > + else > + debug ("Could not find a valid disk image for %s", domain.get_name ()); > + } catch (GLib.Error error) { > + warning ("%s", error.message); > + } > + } > + try { > + yield ensure_disks_readable (disk_paths); > + } catch (GLib.Error error) { > + warning ("Failed to make all libvirt systems disks readable: %s", error.message); > + > + return; > + } > + > debug ("Fetched %u domains from system libvirt.", domains.length ()); > if (domains.length () == 0) > throw new LibvirtSystemImporterError.NO_IMPORTS (_("No boxes to import")); > @@ -49,7 +79,6 @@ public async LibvirtSystemImporter () throws GLib.Error { > public async void import () { > GVirConfig.Domain[] configs = {}; > - string[] disk_paths = {}; > foreach (var domain in domains) { > GVirConfig.Domain config; > @@ -58,19 +87,11 @@ public async void import () { > try { > get_domain_info (domain, out config, out disk_path); > configs += config; > - disk_paths += disk_path; > } catch (GLib.Error error) { > warning ("%s", error.message); > } > } Why not also cache configs at the same time as disk_paths (in constructor) so we don't need to loop over again in here? > - try { > - yield ensure_disks_readable (disk_paths); > - } catch (GLib.Error error) { > - warning ("Failed to make all libvirt system disks readable: %s", error.message); > - > - return; > - } > for (var i = 0; i < configs.length; i++) > import_domain.begin (configs[i], disk_paths[i], null); >
(In reply to Zeeshan Ali (Khattak) from comment #21) > To avoid having to wait on them (they seem unavailable right now), I'll > review here by copy&pasting: > > > @@ -, +, @@ > > src/libvirt-system-importer.vala | 41 ++++++++++++++++++++++++++++++---------- > > 1 file changed, 31 insertions(+), 10 deletions(-) > > --- a/src/libvirt-system-importer.vala > > +++ a/src/libvirt-system-importer.vala > > @@ -9,6 +9,7 @@ > > private class Boxes.LibvirtSystemImporter: GLib.Object { > > private GVir.Connection connection; > > private GLib.List<GVir.Domain> domains; > > + private string[] disk_paths; > > public string wizard_menu_label { > > owned get { > > @@ -41,7 +42,36 @@ > > public async LibvirtSystemImporter () throws GLib.Error { > > connection = yield get_system_virt_connection (); > > - domains = system_virt_connection.get_domains (); > > + var domains_temp = system_virt_connection.get_domains (); > > + domains = new GLib.List<GVir.Domain> (); > > + disk_paths = {}; > > With this disk_path in local scope, the disk_paths you added to class never > gets used. I don't think the actual import will import anything. Same thing > about domains. > > > + > > + foreach (var domain in domains_temp) { > > + try { > > + string disk_path; > > + var config = new GVirConfig.Domain (); > > + > > + get_domain_info (domain, out config, out disk_path); > > + > > + var file = File.new_for_path(disk_path); Lack of spaces again. > > + if(file.query_exists ()){ > > + domains.append(domain); > > + disk_paths+=disk_path; On these two lines too. > > + } > > + else * Else on the same line as the '}' before it. * if 'if' block has {}, so should associated 'else' and 'else if'.
Created attachment 325506 [details] [review] Ignore unimportable VMs early on It may be odd, but actually the disk paths were being properly used in my previous patch. I tested by outputting the data in both the method that gathered them up and in the method used to effectively import the domains. I made the same tests for this patch, but if you think I should change the scope of the local variables in discussion, it's fine with me.
(In reply to Zeeshan Ali (Khattak) from comment #22) > (In reply to Zeeshan Ali (Khattak) from comment #21) > > To avoid having to wait on them (they seem unavailable right now), I'll > > review here by copy&pasting: + disk_paths = {}; > > > > With this disk_path in local scope, the disk_paths you added to class never > > gets used. I don't think the actual import will import anything. Same thing > > about domains. That instruction was in fact redundant, but I researched a little to find out why it worked. The reason is probably that the memory allocated for the list and for each element of the list in particular is still being kept in the memory after the method finishes its execution. Only the references loose scope, but it doesn't trouble us because the references are actually copied in the list/to the list. Correct me if I'm wrong.
Review of attachment 325506 [details] [review]: It seems I accidentally removed an empty line. I'll fix it asap.
Created attachment 325699 [details] [review] Ignore unimportable VMs early on *fixed an empty line
Review of attachment 325699 [details] [review]: The last paragraph is not just vague but also redundant since ' early on' part in previous para implies it clearly. Rest looks good. I'll fix when pushing. ::: src/libvirt-system-importer.vala @@ +12,3 @@ + private string[] disk_paths; + private GVirConfig.Domain[] configs; Actually would make more sense to keep these two lines together with 'domains' declaration above. @@ +45,3 @@ connection = yield get_system_virt_connection (); + var domains_temp = system_virt_connection.get_domains (); You can use 'this' keyword to refer to object's fields/properties to avoid ambiguity instead of having to name local variables differently.
Created attachment 325816 [details] [review] wizard: Ignore unimportable VMs early We currently assume all system libvirt VMs to be importable and end up providing an option to import unimportable VMs and failing to import them when user chooses that option. This looks really bad. Let's check the existence of the VMs' disk images and ensure their readability, early on and simply ignore the VMs without a valid disk image attached to them.
Attachment 325816 [details] pushed as 1521bdb - wizard: Ignore unimportable VMs early