GNOME Bugzilla – Bug 677391
Win7 express install broken due to hostname setting
Last modified: 2016-03-31 13:59:32 UTC
Express installation of Windows 7 is currently broken because of wrong host/computer name setting.
Created attachment 215549 [details] [review] express,win7: Move ComputerName to correct pass
Created attachment 215550 [details] [review] Make use of 'title' node in libvirt XML This is mainly done to fix Windows 7 express installation as it requires the host/computer name to be less than 16 characters[1]. Although we could just work around that issue for Windows 7 but looking at the libvirt documentation[2], the more verbose string we currently use as domain's name is really more like its title and we better use a short name as domain's name in general. [1] http://technet.microsoft.com/en-us/library/cc749460%28v=ws.10%29 [2] http://libvirt.org/formatdomain.html#elementsMetadata
Created attachment 215570 [details] [review] Make use of 'title' node in libvirt XML V2: Don't use spaces in the domain name.
Oh nice, I thought these were my local changes that had broken win7 unattended install ;) ACK to the first patch.
Comment on attachment 215570 [details] [review] Make use of 'title' node in libvirt XML >diff --git a/src/vm-creator.vala b/src/vm-creator.vala >index fba7a8a..329b5c0 100644 >--- a/src/vm-creator.vala >+++ b/src/vm-creator.vala >@@ -43,18 +43,18 @@ private class Boxes.VMCreator { > } > > public async void create_and_launch_vm (InstallerMedia install_media, Cancellable? cancellable) throws GLib.Error { >- var name = yield create_domain_name_from_media (install_media); >+ string title; >+ var name = yield create_domain_name_and_title_from_media (install_media, out title); Returning 'name' and having 'title' an out parameter does not look really nice, can you make 'name' an out arg as well, or make name/title properties of InstallerMedia? > var fullscreen = true; > if (install_media is UnattendedInstaller) { > var unattended = install_media as UnattendedInstaller; > >- var hostname = name.replace (" ", "-"); >- yield unattended.setup (hostname, cancellable); >+ yield unattended.setup (name, cancellable); > fullscreen = !unattended.express_install; > } > > var volume = yield create_target_volume (name, install_media.resources.storage); >- var config = configurator.create_domain_config (install_media, name, volume.get_path ()); >+ var config = configurator.create_domain_config (install_media, name, title, volume.get_path ()); > > var domain = connection.create_domain (config); > domain.start (0); >@@ -139,13 +139,18 @@ private class Boxes.VMCreator { > } > } > >- private async string create_domain_name_from_media (InstallerMedia install_media) throws GLib.Error { >- var base_name = install_media.label; >+ private async string create_domain_name_and_title_from_media (InstallerMedia install_media, >+ out string title) throws GLib.Error { >+ var base_title = install_media.label; >+ title = base_title; >+ var base_name = (install_media.os != null) ? install_media.os.short_id : base_title; > var name = base_name; > > var pool = yield get_storage_pool (); >- for (var i = 2; connection.find_domain_by_name (name) != null || pool.get_volume (name) != null; i++) >- name = base_name + " " + i.to_string (); >+ for (var i = 2; connection.find_domain_by_name (name) != null || pool.get_volume (name) != null; i++) { >+ name = base_name + "-" + i.to_string (); >+ title = base_title + " " + i.to_string (); >+ } If I'm not mistaken, it will be possible to have several boxes with the same title, one for a box created before this patch, and one for a box created after. It's probably better to do some tests on "title" as well.
(In reply to comment #5) > (From update of attachment 215570 [details] [review]) > > > >diff --git a/src/vm-creator.vala b/src/vm-creator.vala > >index fba7a8a..329b5c0 100644 > >--- a/src/vm-creator.vala > >+++ b/src/vm-creator.vala > >@@ -43,18 +43,18 @@ private class Boxes.VMCreator { > > } > > > > public async void create_and_launch_vm (InstallerMedia install_media, Cancellable? cancellable) throws GLib.Error { > >- var name = yield create_domain_name_from_media (install_media); > >+ string title; > >+ var name = yield create_domain_name_and_title_from_media (install_media, out title); > > Returning 'name' and having 'title' an out parameter does not look really nice, > can you make 'name' an out arg as well, or make name/title properties of > InstallerMedia? 'out' arguments are really nothing more than work-arounds for lack of support for multiple return values in languages IMO so while it might not look good, its still correct to return the first/primary value and use 'out' args for the rest. > >+ private async string create_domain_name_and_title_from_media (InstallerMedia install_media, > >+ out string title) throws GLib.Error { > >+ var base_title = install_media.label; > >+ title = base_title; > >+ var base_name = (install_media.os != null) ? install_media.os.short_id : base_title; > > var name = base_name; > > > > var pool = yield get_storage_pool (); > >- for (var i = 2; connection.find_domain_by_name (name) != null || pool.get_volume (name) != null; i++) > >- name = base_name + " " + i.to_string (); > >+ for (var i = 2; connection.find_domain_by_name (name) != null || pool.get_volume (name) != null; i++) { > >+ name = base_name + "-" + i.to_string (); > >+ title = base_title + " " + i.to_string (); > >+ } > > If I'm not mistaken, it will be possible to have several boxes with the same > title, one for a box created before this patch, and one for a box created > after. It's probably better to do some tests on "title" as well. Since title uses the same number as the name, we'll either have collision for both or none unless we change the 'name = base_name + "-"' logic at some point. Keeping in mind that having conflicts in 'title' is not really a big problem, I wouldn't want to spend time on implementing GVir.Connection.find_domain_by_title(). Perhaps a comment in the code would suffice?
(In reply to comment #6) > (In reply to comment #5) > > > > Returning 'name' and having 'title' an out parameter does not look really nice, > > can you make 'name' an out arg as well, or make name/title properties of > > InstallerMedia? > > 'out' arguments are really nothing more than work-arounds for lack of support > for multiple return values in languages IMO so while it might not look good, > its still correct to return the first/primary value and use 'out' args for the > rest. I didn't say it's not correct, all I meant is that it's less readable this way. > > > > If I'm not mistaken, it will be possible to have several boxes with the same > > title, one for a box created before this patch, and one for a box created > > after. It's probably better to do some tests on "title" as well. > > Since title uses the same number as the name, we'll either have collision for > both or none unless we change the 'name = base_name + "-"' logic at some point. This patch is changing the naming logic, so we can get duplicate, that's my point. > Keeping in mind that having conflicts in 'title' is not really a big problem, I > wouldn't want to spend time on implementing > GVir.Connection.find_domain_by_title(). Perhaps a comment in the code would > suffice? Is it a UI/design problem, is it not? I don't know ;) It indeed does not look problematic code-wise.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > > > > Returning 'name' and having 'title' an out parameter does not look really nice, > > > can you make 'name' an out arg as well, or make name/title properties of > > > InstallerMedia? > > > > 'out' arguments are really nothing more than work-arounds for lack of support > > for multiple return values in languages IMO so while it might not look good, > > its still correct to return the first/primary value and use 'out' args for the > > rest. > > I didn't say it's not correct, all I meant is that it's less readable this way. I didn't mean to say that you did. :) I meant more like it seems wrong to me not to use return value for a function that returns multiple values so to me this seems more readable than what you are proposing. > > > > > > If I'm not mistaken, it will be possible to have several boxes with the same > > > title, one for a box created before this patch, and one for a box created > > > after. It's probably better to do some tests on "title" as well. > > > > Since title uses the same number as the name, we'll either have collision for > > both or none unless we change the 'name = base_name + "-"' logic at some point. > > This patch is changing the naming logic, so we can get duplicate, that's my > point. Yeah, I see your point. Just don't feel motivated to solve the issue. :) > > Keeping in mind that having conflicts in 'title' is not really a big problem, I > > wouldn't want to spend time on implementing > > GVir.Connection.find_domain_by_title(). Perhaps a comment in the code would > > suffice? > > Is it a UI/design problem, is it not? I don't know ;) It indeed does not look > problematic code-wise. Keep in mind that I didn't say its *not* a problem but rather that: a. it'll only realize if/when we change the naming logic. We can minimize that risk by adding a comment there. b. even when it happens, it won't cause big issues (no crashes etc). I rest my case now.
(In reply to comment #8) > I didn't mean to say that you did. :) I meant more like it seems wrong to me > not to use return value for a function that returns multiple values so to me > this seems more readable than what you are proposing. > dunno, (return value #1, function name, parameter, return value #2) doesn't flow very well for me, it's likely that I'll miss one of the 2 return values, but I'm fine with blaming you when this happens ;) > > Keep in mind that I didn't say its *not* a problem but rather that: > > a. it'll only realize if/when we change the naming logic. We can minimize that > risk by adding a comment there. > b. even when it happens, it won't cause big issues (no crashes etc). c. if it's a problem UI-wise and gets reported in bugzilla, it's likely to stay there for quite some time before being addressed, that's why I'd prefer if we had a good idea of what we want to do to fix this (if anything) rather than wait and see.
(In reply to comment #9) > (In reply to comment #8) > > > > Keep in mind that I didn't say its *not* a problem but rather that: > > > > a. it'll only realize if/when we change the naming logic. We can minimize that > > risk by adding a comment there. > > b. even when it happens, it won't cause big issues (no crashes etc). > > c. if it's a problem UI-wise and gets reported in bugzilla, it's likely to stay > there for quite some time before being addressed, Why would it take a long time to be addressed? And it *is* a problem UI-wise if/when it happens.
(In reply to comment #10) > Why would it take a long time to be addressed? basing this observation on all the small UI/design bugs that are currently open > And it *is* a problem UI-wise if/when it happens. why "if"? Assuming this patch goes in, someone who installed a Fedora VM with Boxes 3.4 and which install another Fedora VM with Boxes 3.6 will have 2 boxes with the same name.
(In reply to comment #11) > (In reply to comment #10) > > > Why would it take a long time to be addressed? > > basing this observation on all the small UI/design bugs that are currently open > > > And it *is* a problem UI-wise if/when it happens. > > why "if"? Assuming this patch goes in, someone who installed a Fedora VM with > Boxes 3.4 and which install another Fedora VM with Boxes 3.6 will have 2 boxes > with the same name. Ah that we can easily solve by checking if 'title' is being used as name..
(In reply to comment #12) > Ah that we can easily solve by checking if 'title' is being used as name.. Which was my initial (very vague) suggestion ;)
Created attachment 215658 [details] [review] Make use of 'title' node in libvirt XML This version ensures that new title isn't already taken by existing domains. Also added a note about the implication of changing the naming logic.
Comment on attachment 215658 [details] [review] Make use of 'title' node in libvirt XML >From 2a88b833ee8bae5bd34fff5d02e1aa31ac9f075b Mon Sep 17 00:00:00 2001 >From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> >Date: Mon, 4 Jun 2012 16:58:47 +0300 >Subject: [PATCH] Make use of 'title' node in libvirt XML > >This is mainly done to fix Windows 7 express installation as it requires >the host/computer name to be less than 16 characters[1]. Although we >could just work around that issue for Windows 7 but looking at the >libvirt documentation[2], the more verbose string we currently use as >domain's name is really more like its title and we better use a short >name as domain's name in general. > >[1] http://technet.microsoft.com/en-us/library/cc749460%28v=ws.10%29 >[2] http://libvirt.org/formatdomain.html#elementsMetadata > >https://bugzilla.gnome.org/show_bug.cgi?id=677391 >--- > configure.ac | 2 +- > src/collection.vala | 11 +++++++++++ > src/libvirt-machine.vala | 7 ++++--- > src/machine.vala | 4 ++-- > src/vm-configurator.vala | 3 ++- > src/vm-creator.vala | 26 ++++++++++++++++++-------- > 6 files changed, 38 insertions(+), 15 deletions(-) > >diff --git a/configure.ac b/configure.ac >index 238ed20..1d263c5 100644 >--- a/configure.ac >+++ b/configure.ac >@@ -46,7 +46,7 @@ GOBJECT_INTROSPECTION_MIN_VERSION=0.9.6 > GTK_MIN_VERSION=3.3.5 > GTK_VNC_MIN_VERSION=0.4.4 > LIBVIRT_GLIB_MIN_VERSION=0.0.8 >-LIBVIRT_GCONFIG_MIN_VERSION=0.0.8 >+LIBVIRT_GCONFIG_MIN_VERSION=0.0.9 > LIBXML2_MIN_VERSION=2.7.8 > SPICE_GTK_MIN_VERSION=0.9 > GUDEV_MIN_VERSION=165 >diff --git a/src/collection.vala b/src/collection.vala >index a4e328f..f419bf2 100644 >--- a/src/collection.vala >+++ b/src/collection.vala >@@ -2,6 +2,17 @@ > > private abstract class Boxes.CollectionItem: Boxes.UI { > public string name { set; get; } >+ public string title { >+ set { >+ _title = value; >+ } >+ >+ owned get { >+ return _title ?? name; >+ } >+ } >+ >+ private string? _title; > } Why the leading _ ? convention for private members? > > private class Boxes.Collection: GLib.Object { >diff --git a/src/libvirt-machine.vala b/src/libvirt-machine.vala >index 9f991ec..77a7058 100644 >--- a/src/libvirt-machine.vala >+++ b/src/libvirt-machine.vala >@@ -97,9 +97,9 @@ private class Boxes.LibvirtMachine: Boxes.Machine { > } > > public LibvirtMachine (CollectionSource source, >- Boxes.App app, >- GVir.Connection connection, >- GVir.Domain domain) throws GLib.Error { >+ Boxes.App app, >+ GVir.Connection connection, >+ GVir.Domain domain) throws GLib.Error { > base (source, app, domain.get_name ()); > > debug ("new libvirt machine: " + name); >@@ -137,6 +137,7 @@ private class Boxes.LibvirtMachine: Boxes.Machine { > domain.stopped.connect (() => { state = MachineState.STOPPED; }); > > update_domain_config (); >+ title = domain_config.get_title () ?? name; NB: domain_config may probably be null here... > domain.updated.connect (update_domain_config); > > set_screenshot_enable (true); >diff --git a/src/machine.vala b/src/machine.vala >index 80e2cbe..b2f930f 100644 >--- a/src/machine.vala >+++ b/src/machine.vala >@@ -295,9 +295,9 @@ private class Boxes.MachineActor: Boxes.UI { > > gtk_vbox.get_widget ().get_style_context ().add_class ("boxes-bg"); > >- label = new Gtk.Label (machine.name); >+ label = new Gtk.Label (machine.title); > label.modify_fg (Gtk.StateType.NORMAL, get_color ("white")); >- machine.bind_property ("name", label, "label", BindingFlags.DEFAULT); >+ machine.bind_property ("title", label, "label", BindingFlags.DEFAULT); > vbox.add (label); > password_entry = new Gtk.Entry (); > password_entry.set_visibility (false); >diff --git a/src/vm-configurator.vala b/src/vm-configurator.vala >index 0895f2e..252ea28 100644 >--- a/src/vm-configurator.vala >+++ b/src/vm-configurator.vala >@@ -13,9 +13,10 @@ private class Boxes.VMConfigurator { > private const string INSTALLATION_XML = "<os-state>" + INSTALLATION_STATE + "</os-state>"; > private const string INSTALLED_XML = "<os-state>" + INSTALLED_STATE + "</os-state>"; > >- public Domain create_domain_config (InstallerMedia install_media, string name, string target_path) { >+ public Domain create_domain_config (InstallerMedia install_media, string name, string title, string target_path) { > var domain = new Domain (); > domain.name = name; >+ domain.title = title; I'm not a big fan of functions with many args, the caller could set name and title on the resulting Domain object, similar to gtk_window_new/gtk_window_set_title. I think 'display_name' would be more descriptive than 'title' throughout this patch > > var xml = (install_media.live) ? LIVE_XML : INSTALLATION_XML; > >diff --git a/src/vm-creator.vala b/src/vm-creator.vala >index fba7a8a..58d9a73 100644 >--- a/src/vm-creator.vala >+++ b/src/vm-creator.vala >@@ -43,18 +43,18 @@ private class Boxes.VMCreator { > } > > public async void create_and_launch_vm (InstallerMedia install_media, Cancellable? cancellable) throws GLib.Error { >- var name = yield create_domain_name_from_media (install_media); >+ string title; >+ var name = yield create_domain_name_and_title_from_media (install_media, out title); > var fullscreen = true; > if (install_media is UnattendedInstaller) { > var unattended = install_media as UnattendedInstaller; > >- var hostname = name.replace (" ", "-"); >- yield unattended.setup (hostname, cancellable); >+ yield unattended.setup (name, cancellable); > fullscreen = !unattended.express_install; > } > > var volume = yield create_target_volume (name, install_media.resources.storage); >- var config = configurator.create_domain_config (install_media, name, volume.get_path ()); >+ var config = configurator.create_domain_config (install_media, name, title, volume.get_path ()); > > var domain = connection.create_domain (config); > domain.start (0); >@@ -139,13 +139,23 @@ private class Boxes.VMCreator { > } > } > >- private async string create_domain_name_from_media (InstallerMedia install_media) throws GLib.Error { >- var base_name = install_media.label; >+ private async string create_domain_name_and_title_from_media (InstallerMedia install_media, >+ out string title) throws GLib.Error { >+ var base_title = install_media.label; >+ title = base_title; >+ var base_name = (install_media.os != null) ? install_media.os.short_id : base_title; > var name = base_name; > > var pool = yield get_storage_pool (); >- for (var i = 2; connection.find_domain_by_name (name) != null || pool.get_volume (name) != null; i++) >- name = base_name + " " + i.to_string (); >+ for (var i = 2; >+ connection.find_domain_by_name (name) != null || >+ connection.find_domain_by_name (title) != null || A comment explaining that this find_domain_by_name(title) call is there because in 3.4 we were using 'title' as the VM name, and that without this test we may get duplicate names would be good for future code archeologists. >+ pool.get_volume (name) != null; >+ i++) { >+ // If you change the naming logic, you must address the issue of duplicate titles you'll be introducing >+ name = base_name + "-" + i.to_string (); >+ title = base_title + " " + i.to_string (); >+ } > > return name; > } >-- >1.7.10.2
(In reply to comment #15) > >+ private string? _title; > > } > > Why the leading _ ? convention for private members? For members that are simply place-holder for a property of the same name. > > update_domain_config (); > >+ title = domain_config.get_title () ?? name; > > NB: domain_config may probably be null here... How? update_domain_config synchronously fetches the domain config before this call. > >- public Domain create_domain_config (InstallerMedia install_media, string name, string target_path) { > >+ public Domain create_domain_config (InstallerMedia install_media, string name, string title, string target_path) { > > var domain = new Domain (); > > domain.name = name; > >+ domain.title = title; > > I'm not a big fan of functions with many args, the caller could set name and > title on the resulting Domain object, similar to > gtk_window_new/gtk_window_set_title. Its only 4 arguments but yeah, I could actually do that for name as well. > I think 'display_name' would be more descriptive than 'title' throughout this > patch To me 'title' vs 'name' is pretty much self-explanatory, especially when libvirt also uses 'title'.
(In reply to comment #16) > (In reply to comment #15) > > NB: domain_config may probably be null here... > > How? update_domain_config synchronously fetches the domain config before this > call. It tries to fetch the domain config, but silently eats away errors that may happen during the fetching.
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > > NB: domain_config may probably be null here... > > > > How? update_domain_config synchronously fetches the domain config before this > > call. > > It tries to fetch the domain config, but silently eats away errors that may > happen during the fetching. It sends out a critical and AFAIK this only fails when something has terribly gone wrong so that we are totally scr**d anyways if this happens. You'll see that in rest of the (existing) code, its also assumed that update_domain_config() always succeeds.
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > (In reply to comment #15) > > > > > > NB: domain_config may probably be null here... > > > > > > How? update_domain_config synchronously fetches the domain config before this > > > call. > > > > It tries to fetch the domain config, but silently eats away errors that may > > happen during the fetching. > > It sends out a critical and AFAIK this only fails when something has terribly > gone wrong so that we are totally scr**d anyways if this happens. You'll see > that in rest of the (existing) code, its also assumed that > update_domain_config() always succeeds. I know there is more broken code that needs fixing, that's why I put that comment in a NB: Given that there's already a crash reported because we're careless with that function, that's worth at least mentioning it.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > (In reply to comment #16) > > > > (In reply to comment #15) > > > > > > > > NB: domain_config may probably be null here... > > > > > > > > How? update_domain_config synchronously fetches the domain config before this > > > > call. > > > > > > It tries to fetch the domain config, but silently eats away errors that may > > > happen during the fetching. > > > > It sends out a critical and AFAIK this only fails when something has terribly > > gone wrong so that we are totally scr**d anyways if this happens. You'll see > > that in rest of the (existing) code, its also assumed that > > update_domain_config() always succeeds. > > I know there is more broken code that needs fixing, that's why I put that > comment in a NB: > Given that there's already a crash reported because we're careless with that > function, that's worth at least mentioning it. I totally disagree with you assessment here. That crash report exists only because we leave a half-baked domain around and that would nicely go under 'this only fails when something has terribly gone wrong so that we are totally scr**d anyways'. IMO We must do everything to prevent such situation to arise in the first place rather than checking each and every assumption of ours at every point in the code.
(In reply to comment #20) > We must do everything to prevent such situation to arise > in the first place rather than checking each and every assumption of ours at > every point in the code. This probably can be triggered by the user killing libvirtd (yes, stupid idea). When this happens, I'd much prefer boxes to stay alive even if it cannot do anything rather than having it crash right away (though in such cases we can probably reconnect right away). Not checking the return value of a function that can return NULL on failure (especially when this failure is not under our control) is *bad* With vala supporting exceptions, it shouldn't be too hard to propagate that error...
(In reply to comment #21) > (In reply to comment #20) > > We must do everything to prevent such situation to arise > > in the first place rather than checking each and every assumption of ours at > > every point in the code. > > This probably can be triggered by the user killing libvirtd (yes, stupid idea). > When this happens, I'd much prefer boxes to stay alive even if it cannot do > anything rather than having it crash right away (though in such cases we can > probably reconnect right away). Actually I'd prefer punishing user for fiddling with things she is not supposed to even know about. However, I won't mind if you try to fix this either. As long as we don't go too far with this..
Created attachment 215792 [details] [review] Make use of 'title' node in libvirt XML V4: * Less arguments to VMConfigurator.create_domain_config() * Actually use title in the collection view. NB: In defense of using 'title' rather than 'display_name', it seems that CollectionView already uses this term as well
Comment on attachment 215792 [details] [review] Make use of 'title' node in libvirt XML >From 6af3941f3e584206be31123b061705fc06bbc5f1 Mon Sep 17 00:00:00 2001 >From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> >Date: Mon, 4 Jun 2012 16:58:47 +0300 >Subject: [PATCH] Make use of 'title' node in libvirt XML > >This is mainly done to fix Windows 7 express installation as it requires >the host/computer name to be less than 16 characters[1]. Since I don't think there is any guarantee at all that os.short_id is less than 16 characters, can you at least add a test for the length to the win7 code and output a g_warning if we hit this (or any other way of your choosing of logging this)? >- private async string create_domain_name_from_media (InstallerMedia install_media) throws GLib.Error { >- for (var i = 2; connection.find_domain_by_name (name) != null || pool.get_volume (name) != null; i++) >- name = base_name + " " + i.to_string (); >+ for (var i = 2; connection.find_domain_by_name (name) != null || pool.get_volume (name) != null; i++) { >+ name = base_name + "-" + i.to_string (); >+ title = base_title + " " + i.to_string (); >+ } Any reason for dropping the comment and the connection.find_domain_by_name (title) call from the last patch? Looks good otherwise.
(In reply to comment #24) > (From update of attachment 215792 [details] [review]) > >From 6af3941f3e584206be31123b061705fc06bbc5f1 Mon Sep 17 00:00:00 2001 > >From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> > >Date: Mon, 4 Jun 2012 16:58:47 +0300 > >Subject: [PATCH] Make use of 'title' node in libvirt XML > > > >This is mainly done to fix Windows 7 express installation as it requires > >the host/computer name to be less than 16 characters[1]. > > Since I don't think there is any guarantee at all that os.short_id is less than > 16 characters, can you at least add a test for the length to the win7 code and > output a g_warning if we hit this (or any other way of your choosing of logging > this)? Good point! I'll do that. > >- private async string create_domain_name_from_media (InstallerMedia install_media) throws GLib.Error { > >- for (var i = 2; connection.find_domain_by_name (name) != null || pool.get_volume (name) != null; i++) > >- name = base_name + " " + i.to_string (); > >+ for (var i = 2; connection.find_domain_by_name (name) != null || pool.get_volume (name) != null; i++) { > >+ name = base_name + "-" + i.to_string (); > >+ title = base_title + " " + i.to_string (); > >+ } > > Any reason for dropping the comment and the connection.find_domain_by_name > (title) call from the last patch? Yikes, I messed-up the rebase.. Will fix this too..
(In reply to comment #20) > I totally disagree with you assessment here. That crash report exists only > because we leave a half-baked domain around and that would nicely go under > 'this only fails when something has terribly gone wrong so that we are totally > scr**d anyways'. IMO We must do everything to prevent such situation to arise > in the first place rather than checking each and every assumption of ours at > every point in the code. I agree with zeeshan that situation that should not happen in the first place should be treated like assert() or can just critical/crash. If libvirt dies, the X server dies or we are OOM, we decided not to survive these kind of cases. However, if a VM dies, a Spice connection dies, or something that is specific to a particual element/usage in the application (as opposed to the whole application in general), then it is better to survive. Even though it wouldn't be critical to crash in the cases of Boxes (99% of state is saved in VM & libvirt) Imo, if in the future libvirt is one among several ways to manage the VM, then we could try to be safe if libvirt crash, for now it's not a problem since Boxes is pretty useless without it.
(In reply to comment #26) > > I agree with zeeshan that situation that should not happen in the first place > should be treated like assert() or can just critical/crash. If libvirt dies, > the X server dies or we are OOM, we decided not to survive these kind of cases. Then let's be honest about it and put an assert in the right place(s). The fact that we already had such a crash makes me quite sure that we'll hit this again in the future, which is why I'd rather have this handled more nicely than with an assert...
Created attachment 215887 [details] [review] Make use of 'title' node in libvirt XML * Re-add accidently dropped code and comments from previous revision * Add comment about why we check for domain by name based on title * Add critical log message if OS's short ID somehow goes bigger than 15 characters.
Created attachment 215912 [details] [review] Use g_utf8_casefold to compare boxes titles A case insensitive comparison is made to order the boxes in the collection view. This comparison is currently done with g_utf8_strdown. At this point, the box names only contain 7bit ASCII characters so this is probably OK, however this will no longer be a safe assumption when the box titles can be changed by the user. This commit uses g_utf8_casefold instead to improve case insensitive ordering of arbitrary box titles.
The patch above is a small unrelated improvement I noticed when looking at your 2nd patch.
Created attachment 215913 [details] [review] Abort when we couldn't fetch the libvirt config As per the reasoning on https://bugzilla.gnome.org/show_bug.cgi?id=677391#c26 « Marc-Andre Lureau [developer] 2012-06-07 13:37:27 UTC (In reply to comment #20) > I totally disagree with you assessment here. That crash report exists only > because we leave a half-baked domain around and that would nicely go under > 'this only fails when something has terribly gone wrong so that we are totally > scr**d anyways'. IMO We must do everything to prevent such situation to arise > in the first place rather than checking each and every assumption of ours at > every point in the code. I agree with zeeshan that situation that should not happen in the first place should be treated like assert() or can just critical/crash. If libvirt dies, the X server dies or we are OOM, we decided not to survive these kind of cases. However, if a VM dies, a Spice connection dies, or something that is specific to a particual element/usage in the application (as opposed to the whole application in general), then it is better to survive. Even though it wouldn't be critical to crash in the cases of Boxes (99% of state is saved in VM & libvirt) Imo, if in the future libvirt is one among several ways to manage the VM, then we could try to be safe if libvirt crash, for now it's not a problem since Boxes is pretty useless without it. »
Review of attachment 215913 [details] [review]: This is specific to a particular domain/vm. Hence, critical is better. If libvirt == null, this is general failure.
(In reply to comment #32) > Review of attachment 215913 [details] [review]: > > This is specific to a particular domain/vm. Hence, critical is better. If > libvirt == null, this is general failure. Well, there's a reported crash because domain_config ends up being NULL after calling this function, and the consensus in this bug seemed to be that we didn't care because this should not happen, hence the g_error.
(In reply to comment #33) > Well, there's a reported crash because domain_config ends up being NULL after > calling this function, and the consensus in this bug seemed to be that we > didn't care because this should not happen, hence the g_error. The domain may have been delete before/while we made the request. We should probably handle that case correctly.
Review of attachment 215912 [details] [review]: Looks right otherwise. ::: src/collection-view.vala @@ -182,3 +182,3 @@ return 0; - return strcmp (item_a.title.down (), item_b.title.down ()); + return strcmp (item_a.title.casefold (), item_b.title.casefold ()); Shouldn't we be using string.collate() instead of strcmp?
Review of attachment 215549 [details] [review]: Pushed!
Review of attachment 215887 [details] [review]: Pushed!
Review of attachment 215912 [details] [review]: ::: src/collection-view.vala @@ -182,3 +182,3 @@ return 0; - return strcmp (item_a.title.down (), item_b.title.down ()); + return strcmp (item_a.title.casefold (), item_b.title.casefold ()); Ah that's likely, thanks. However, if we use .collate(), I'd drop the casefold() since I'd expect .collate to do that for us if that's what is expected in the current locale: return item_a.title.collate (item_b.title);
(In reply to comment #38) > Review of attachment 215912 [details] [review]: > > ::: src/collection-view.vala > @@ -182,3 +182,3 @@ > return 0; > > - return strcmp (item_a.title.down (), item_b.title.down ()); > + return strcmp (item_a.title.casefold (), item_b.title.casefold > ()); > > Ah that's likely, thanks. However, if we use .collate(), I'd drop the > casefold() since I'd expect .collate to do that for us if that's what is > expected in the current locale: > return item_a.title.collate (item_b.title); Yeah, if I understand the g_utf8_collate correctly.
Created attachment 215970 [details] [review] Use g_utf8_collate when sorting boxes Sort the list of boxes shown to the user according to the sorting rules in use for his locale. For now this does not really matter since boxes names are only 7bit ASCII chars, but this will become an issue once the user can name his boxes.
Review of attachment 215970 [details] [review]: ACK, assuming you tested.
Comment on attachment 215970 [details] [review] Use g_utf8_collate when sorting boxes Attachment 215970 [details] pushed as 169b5a9 - Use g_utf8_collate when sorting boxes
Review of attachment 215887 [details] [review]: ::: src/collection.vala @@ +13,3 @@ + } + + private string? _title; This "title" property is very much specific to libvirt-machine. "name" is and should stay free-form, user editable. Until now, it has no limitation of length or content. I suspect the way it is used in the unattended win7 could be corrected. Now, we have 2 fields that make it hard to know what is used for what honestly, and if it's a libvirt or win7 specific work around, we need to push it to libvirt specific code instead, and not complicate base code and base classes. The CollectionItem very generic, it could even have no "name" property eventually. It can be any kind of object, being a machine, or a folder, or another object. Having free-form "name" only is simple enough to look it up by name, sort, or display it in a list. There should be no need for title there.
see bug 680168 for the libvirt "title" vs item "name"
(In reply to comment #43) > Review of attachment 215887 [details] [review]: > > ::: src/collection.vala > @@ +13,3 @@ > + } > + > + private string? _title; > > This "title" property is very much specific to libvirt-machine. > > "name" is and should stay free-form, user editable. Until now, it has no > limitation of length or content. > > I suspect the way it is used in the unattended win7 could be corrected. > > Now, we have 2 fields that make it hard to know what is used for what honestly, > and if it's a libvirt or win7 specific work around, we need to push it to > libvirt specific code instead, and not complicate base code and base classes. What we need is a name (free from, descriptive), and a shortname (which is used to generate a win7 hostname since this must be short, but I can see how this can be useful for other stuff, for example for filenames). I don't think this requirement is libvirt specific either. I agree that name VS title is confusing though, took me a while to remember the whole story. Would using name/shortname as the naming be good for you?
What is libvirt specific though is the whole box creation process, so I can see your point about 'title' being libvirt-specific.
(In reply to comment #45) > What we need is a name (free from, descriptive), and a shortname (which is used > to generate a win7 hostname since this must be short, but I can see how this > can be useful for other stuff, for example for filenames). I don't think this > requirement is libvirt specific either. I agree that name VS title is confusing > though, took me a while to remember the whole story. Would using name/shortname > as the naming be good for you? I don't see why here. if you need a shortname for libvirt or unattended installation, you can do it there. There is no clear need for a common short name atm. And it would have to be in Machine, not in the base collection item. See the patch in bug 680168, it's not necessary, and we can keep using item name like it was before. The various files generated from a machine/VM don't need to be derived from its name. In fact, using UUID is preferable for instance, and that's what we do for screenshot, as it is still valid after VM rename and is more filesystem friendly etc.
The original bug 'Win7 express install broken due to hostname setting' is fixed and all patches here are pushed so should we close this?
Makes sense to me.