GNOME Bugzilla – Bug 679752
Use viostor drivers during windows installation
Last modified: 2016-03-31 14:00:16 UTC
Here is the patch series to use the virtio drivers during windows installation so that guests use a virtio disk controller. This relies on an external iso being available with the drivers. It can be found at http://teuf.fedorapeople.org/boxes/extra-iso/win-tools.iso and needs to be manually put to ~/.cache/gnome-boxes. It's also looked for in $datadir/gnome-boxes so that we can ship it in an rpm for easier distribution. I've tested that unattended installs work with and without this iso.
Created attachment 218544 [details] [review] Rework UnattendedFile interface Don't hardcode in the base interface that UnattendedFile subclasses will use a temporary file. Given that the various UnattendedInstaller instances stick around forever, we have to make sure UnattendedInstaller::unattended_files is emptied once the files are copied so that their destructors run to do the cleanup. https://bugzilla.gnome.org/show_bug.cgi?id=677038
Created attachment 218545 [details] [review] Add UnattendedRawFile class https://bugzilla.gnome.org/show_bug.cgi?id=677038
Created attachment 218546 [details] [review] Add an argument to Util::run_in_thread() This makes it possible to pass an argument to the function to be run in a separate thread.
Created attachment 218547 [details] [review] Add hack to workaround broken IOError.from_errno g_io_error_from_errno returns a GIOErrorEnum, and vala doesn't know it should convert it to a GIOError *, which causes compilation issues with: // valac-0.16 --pkg gio-2.0 foo.vala // /home/teuf/jhbuild-boxes/sources/gnome-boxes/foo.vala.c: In function ‘test’: // /home/teuf/jhbuild-boxes/sources/gnome-boxes/foo.vala.c:26:9: erreur: // incompatible types when assigning to type ‘struct GError *’ from type ‘GIOErrorEnum’ // error: cc exited with status 26 // Compilation failed: 1 error(s), 0 warning(s) void test () throws GLib.Error { throw GLib.IOError.from_errno(22); // EINVAL } int main () { try { test (); } catch (GLib.Error e) {} return 0; }
Created attachment 218548 [details] [review] Add FuseIso class It's used to mount an iso image and to read some content from it. Boxes will use it to get the extra drivers to copy to the winxp unattended install floppy.
Created attachment 218549 [details] [review] Use FuseIso in FedoraInstaller This removes some duplicated code between FedoraInstaller and FuseIso
Created attachment 218550 [details] [review] Insert extra iso if available This iso will be used during automatic Windows installations. It contains the viostor driver to be able to use virtio at install time and extra drivers (vioserial, ...) and utilities (vdagent) which are added to the install after it's done. This improves VM integration a lot. The name of the iso is win-tools.iso, and it's currently looked up in $XDG_CACHE_HOME/gnome-boxes/ first, and if not found, it's searched in $datadir/gnome-boxes/unattended.
Created attachment 218551 [details] [review] Add InstallerMedia::supports_virtio_disk method This will be useful to be able to automatically use the viostor driver on Windows when we have it available
Created attachment 218552 [details] [review] win7: use drivers from extra iso at install time If the extra driver iso is available when creating a new win7 box, we can use these drivers during installation to be able to use VirtIO through viostor during the installation, which is desirable for better performance. If the ISO is not available, this entry is silently ignored, so it doesn't hurt to have it always there.
Created attachment 218553 [details] [review] winxp: copy drivers from the extra install ISO Boxes optionally supports using an additional ISO image with extra drivers to install to get better integration after the installation. However, Windows XP (contrary to Windows 7) is not able to read drivers directly from the CD but needs them to be copied to the unattended floppy image.
Created attachment 218554 [details] [review] win:use viostor in unattended installs if possible An extra ISO with the additional drivers need to be available, and Boxes needs to be able to locate it.
(In reply to comment #0) > Here is the patch series to use the virtio drivers during windows > installation so that guests use a virtio disk controller. This > relies on an external iso being available with the drivers. It > can be found at http://teuf.fedorapeople.org/boxes/extra-iso/win-tools.iso > and needs to be manually put to ~/.cache/gnome-boxes. It's also > looked for in $datadir/gnome-boxes so that we can ship it in an > rpm for easier distribution. > I've tested that unattended installs work with and without this iso. Can't this ISO be generated as part of the build using genisoimage ?
Review of attachment 218544 [details] [review]: Instead of attaching the same patch in two bugs, I suggest you add a dep to 677038 instead.
Review of attachment 218545 [details] [review]: Same here
Review of attachment 218546 [details] [review]: I don't think we need this. To pass data around in vala, you either use lambdas+closure or pass instance methods that keeps the instance and therefore all its state with it.
Review of attachment 218547 [details] [review]: Why not fix the biding instead?
Review of attachment 218547 [details] [review]: I meant 'binding'.
Review of attachment 218548 [details] [review]: ::: src/fuse-iso.vala @@ +2,3 @@ + +// Helper class to extract files from an ISO image +private class Boxes.FuseIso: GLib.Object { I'd name it ISOExtractor (or some other generic name) rather as use of FUSE is internal detail of this class. @@ +14,3 @@ + debug ("Unmounting '%s'..", mount_path); + string[] argv = { "fusermount", "-u", mount_path }; + exec (argv, null); * exec is async so you want to yield here and make unmount_in_thread async. * there is no need for '_in_thread' suffix, especially after its declared async. @@ +25,3 @@ + return; + + run_in_thread.begin (unmount_in_thread, mount_point.get_path () ); Once unmount_in_thread is made async, you don't need to use run_in_thread here and just do: unmount.begin (mount_point.get_path ()); @@ +50,3 @@ + throw new GLib.IOError.NOT_MOUNTED(""); + + string abs_src_path = Path.build_filename(mount_point.get_path (), rel_path); coding-style nitpicks: * use 'var' wherever possible for local variables. * space before '('. @@ +55,3 @@ + } + + public string get_absolute_path (string rel_path) throws GLib.Error { IMHO: rel_path -> relative_path @@ +57,3 @@ + public string get_absolute_path (string rel_path) throws GLib.Error { + if (!mounted) + throw new GLib.IOError.NOT_MOUNTED(""); * coding-style nitpick: space before '(' * I'll rather use 'requires' here as this method shouldn't be called before mount_media and after unmount_media (i-e programmer error, not runtime). @@ +59,3 @@ + throw new GLib.IOError.NOT_MOUNTED(""); + + return Path.build_filename(mount_point.get_path (), rel_path); coding-style nitpick: space before '('
Review of attachment 218549 [details] [review]: The approach is nice. Apart from specific comments below, I think this should be squashed into the parent patch as we are essentially moving existing code into a separate class and modifying it accordingly. ::: src/fedora-installer.vala @@ +59,3 @@ + public override async void setup (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (express_toggle.active) { coding-style nitpick: No curly braces around this 'if' and its 'else' needed. @@ +61,3 @@ + if (express_toggle.active) { + if (os_media.kernel_path != null && os_media.initrd_path != null) { + FuseIso fuse = new FuseIso (device_file); coding-style nitpick: use var @@ -77,1 +69,2 @@ - yield normal_clean_up (cancellable); + // the ISO will be unmounted once this block ends, so we need + // to make sure we are done with the setup before it's done As far as I can tell, base.setup doesn't need the mounted media @@ +69,3 @@ + // the ISO will be unmounted once this block ends, so we need + // to make sure we are done with the setup before it's done + yield base.setup (vm_name, cancellable); * Do we really need this at the end? * We are making the same call in the else section below. Better just call it outside the 'if' and remove the 'else'. @@ -154,3 +110,3 @@ } - private async File copy_file (File file, string dest_path, Cancellable? cancellable) throws GLib.Error { + private async File copy_file (string src_path, string dest_path, Cancellable? cancellable) throws GLib.Error { this change seems unrelated to this patch so maybe put it as a separate patch?
Review of attachment 218550 [details] [review]: As said in a previous comment, would be nice to be able to generate it as part of build. If thats not feasible/possible, we should look into making this ISO available in a canonical location (somewhere download.gnome.org perhaps?) and then (optionally) downloading it as part of the build. There is also minor coding-style issues that I'd point out later if still present.. ::: src/vm-configurator.vala @@ -222,0 +230,5 @@ + private static void set_extra_config (Domain domain, InstallerMedia install_media) { + /* Allows us to add an extra install media, for example with more + * windows drivers ... 2 more ... Pretty sure you know already but just to be sure, we wouldn't want to do this check anymore after you have rebased your changes on today's git master. Also as part of that, this whole method should be moved to InstallerMedia. ::: src/win7-installer.vala @@ -46,2 +46,4 @@ os.short_id, os.name); + + extra_iso = lookup_extra_iso ("win-tools.iso"); I'd rather subclasses provide the name of ISO to look for (by setting a field in UnattendedInstaller) in their constructors/constructor methods and lookup_extra_iso remains private to UnattendedInstaller.
Review of attachment 218551 [details] [review]: I was a bit confused at first as I was sure we do this already. You might want to make it clear in the log that you are only making the check more available. ::: src/vm-configurator.vala @@ +193,3 @@ + + if (install_media.supports_virtio_disk ()) { + debug("Using virtio controller for the install disk"); coding-style: No space before '('. Since this seems to be in most of your patches, I'll stop pointing it out now and trust you to fix this everywhere..
Review of attachment 218551 [details] [review]: Wrong status, there were still the coding-style issues..
Review of attachment 218552 [details] [review]: Looks right from what I can tell.
Review of attachment 218553 [details] [review]: Looks good otherwise. ::: src/winxp-installer.vala @@ +45,3 @@ + yield fuse.mount_media (cancellable); + + FileInfo info; no need for separate declaration, just do 'var info =' in the loop below.. @@ +51,3 @@ + else + driver_path = "winxp/x86"; + GLib.FileEnumerator enumerator = yield fuse.enumerate_children (driver_path); coding-style: use of 'var' @@ -41,0 +41,21 @@ + public override async void setup (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { + FuseIso fuse = new FuseIso (extra_iso); ... 18 more ... Some of the comments to 'Use FuseIso in FedoraInstaller' patch apply here as well I think.
Review of attachment 218554 [details] [review]: Message from commit log nazis: Space after ":" and Capitalize first letter (u). ::: src/winxp-installer.vala @@ -41,2 +47,2 @@ public override async void setup (string vm_name, Cancellable? cancellable) throws GLib.Error { - if (extra_iso != null) { + bool have_viostor = false; so if I understood correctly, you ensure that in case of XP the drivers present on the custom ISO but not in case of win7? @@ -42,4 +48,4 @@ - if (extra_iso != null) { - FuseIso fuse = new FuseIso (extra_iso); - - yield fuse.mount_media (cancellable); + bool have_viostor = false; + has_viostor_drivers = false; + + try { Would want to avoid putting the whole block under one big try. If extra_iso is provided, it must contain the drivers. Since we are providing plenty of trace of whats going on with debug() and setup() is expected to throw errors if something goes wrong, I'd just let this fail and keep the code simple and readable.. @@ +69,3 @@ + add_unattended_file (new UnattendedRawFile (this, full_path, info.get_name ())); + if (info.get_name () == "viostor.sys") + have_viostor = true; I think you want break the loop here. @@ +78,2 @@ } + // if we went there with no exception, then everything is setup for went there -> arrived here :)
Review of attachment 218548 [details] [review]: ::: src/fuse-iso.vala @@ +50,3 @@ + throw new GLib.IOError.NOT_MOUNTED(""); + + string abs_src_path = Path.build_filename(mount_point.get_path (), rel_path); I use var for local variable when their type is obvious by just looking at the variable initialization (eg var my_var = new Foo ()). When the type is non-obvious, I much prefer using the explicit type. This way, the type of the variable is immediatly known instead of having to dig through function prototypes to find the actual type. This makes the code easier to read and hack on in the future.
Review of attachment 218545 [details] [review]: Nope, this one's not the same, we agreed to drop this patch from the other bug and to have it as part of the series that would make use of it.
Review of attachment 218544 [details] [review]: ok, I never know what to do in these cases ;) I attached it here since it makes applying the series easier, but I'm fine with just having it in the other bug.
Review of attachment 218547 [details] [review]: I'll improve the commit log. Juerg suggested this workaround after saying that fixing this properly required more work.
Review of attachment 218546 [details] [review]: Need to improve commit log here as well. That's indeed what you usually do, except when you hit vala bugs (#677261). After wasting more than half a day on this, Alex suggested this workaround which helped. That said, if exec can be used async as you suggest in the later patches, this change becomes unneeded
Review of attachment 218549 [details] [review]: Not sure about squashing the 2, I'm a bit worried this will make things harder to read since the patch isn't that small. No strong feeling either way, I'll look at squashing this. ::: src/fedora-installer.vala @@ -77,1 +69,2 @@ - yield normal_clean_up (cancellable); + // the ISO will be unmounted once this block ends, so we need + // to make sure we are done with the setup before it's done Ah, good point, I'll look more closely, but it's probably just a blind 'copy' of what I did in the windows code. @@ -154,3 +110,3 @@ } - private async File copy_file (File file, string dest_path, Cancellable? cancellable) throws GLib.Error { + private async File copy_file (string src_path, string dest_path, Cancellable? cancellable) throws GLib.Error { Yep, I can split it, it's a small code simplification but is not really related to this patch
Review of attachment 218551 [details] [review]: ::: src/vm-configurator.vala @@ +193,3 @@ + + if (install_media.supports_virtio_disk ()) { + debug("Using virtio controller for the install disk"); I'll try, no promises I won't miss any... ;)
(In reply to comment #12) > (In reply to comment #0) > > Here is the patch series to use the virtio drivers during windows > > installation so that guests use a virtio disk controller. This > > relies on an external iso being available with the drivers. It > > can be found at http://teuf.fedorapeople.org/boxes/extra-iso/win-tools.iso > > and needs to be manually put to ~/.cache/gnome-boxes. It's also > > looked for in $datadir/gnome-boxes so that we can ship it in an > > rpm for easier distribution. > > I've tested that unattended installs work with and without this iso. > > Can't this ISO be generated as part of the build using genisoimage ? Long term, maybe. The current iso is just something very alpha that I uploaded for testing. It's built by hand from various bits and pieces, and I'm planning on automating this when I get a chance, and then think about how to best distribute this. Having Boxes build it is something I hadn't thought about, but this will probably not be practical.
Review of attachment 218548 [details] [review]: ::: src/fuse-iso.vala @@ +50,3 @@ + throw new GLib.IOError.NOT_MOUNTED(""); + + string abs_src_path = Path.build_filename(mount_point.get_path (), rel_path); But thats not what we have been following uptil now so please follow the coding-style we already have until you a. convince us all about this b. you change it all over the code for us. Nothing is more unreadable than inconsistent coding-style in one project.
Review of attachment 218545 [details] [review]: oh, sounded very familiar. :) Sorry.
Review of attachment 218547 [details] [review]: Of course it involves some work but how much? I'd guess its easily solvable using custom bindings but I'll ask juergbi to be sure.
(In reply to comment #37) > Review of attachment 218547 [details] [review]: > > Of course it involves some work but how much? I'd guess its easily solvable > using custom bindings but I'll ask juergbi to be sure. Not a vala developer, not really interested in that ;) Juerg suggested that I use this workaround, which I took as implicit blessing from upstream vala for using this workaround in apps, enough for me.
Review of attachment 218548 [details] [review]: ::: src/fuse-iso.vala @@ +50,3 @@ + throw new GLib.IOError.NOT_MOUNTED(""); + + string abs_src_path = Path.build_filename(mount_point.get_path (), rel_path); For a), I've already shared my reasoning, if you think it's better the way it currently is, it's your turn to explain why you disagree it's better, if you do. In this very specific case, I don't think changing the whole code base is worth it since I don't think mixed var use/non-var use don't really hurt readibility imo.
Review of attachment 218548 [details] [review]: ::: src/fuse-iso.vala @@ +25,3 @@ + return; + + run_in_thread.begin (unmount_in_thread, mount_point.get_path () ); Hmm, all of this is very interesting, I hadn't thought of things this way. However, this means the final unlink will run in the mainloop thread, which is one of the things I wanted to avoid, so I'd favour keeping the current code.
Review of attachment 218548 [details] [review]: ::: src/fuse-iso.vala @@ +50,3 @@ + throw new GLib.IOError.NOT_MOUNTED(""); + + string abs_src_path = Path.build_filename(mount_point.get_path (), rel_path); This is not how things are done. Talk to me and elmarco (not here) and once you have convinced us, you *must* change it everyone before it can become the new coding-style.
(In reply to comment #38) > (In reply to comment #37) > > Review of attachment 218547 [details] [review] [details]: > > > > Of course it involves some work but how much? I'd guess its easily solvable > > using custom bindings but I'll ask juergbi to be sure. > > Not a vala developer, not really interested in that ;) If you are going to submit patches to Boxes, I'm not sure how you can stay around from becoming a Vala developer. :) > Juerg suggested that I > use this workaround, which I took as implicit blessing from upstream vala for > using this workaround in apps, enough for me. Turns out that this isn't at all trivial fix. You want to link to the bug in your comment btw: https://bugzilla.gnome.org/show_bug.cgi?id=664530
Review of attachment 218549 [details] [review]: ::: src/fedora-installer.vala @@ +59,3 @@ + public override async void setup (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (express_toggle.active) { if (xx) if (yy) foo() else bar() is not the same as if (xx) { if (yy) foo() } else { bar() } I'll remove them from the else @@ +69,3 @@ + // the ISO will be unmounted once this block ends, so we need + // to make sure we are done with the setup before it's done + yield base.setup (vm_name, cancellable); well it's if (A) { if (B) yield base.setup () else ; //do nothing } else yield base.setup () I'll check why there is one case when it's not called, and if it's really necessary to do things this way, I'll add an empty else block with an explanation for more clarity.
Review of attachment 218549 [details] [review]: ::: src/fedora-installer.vala @@ +69,3 @@ + // the ISO will be unmounted once this block ends, so we need + // to make sure we are done with the setup before it's done + yield base.setup (vm_name, cancellable); I think I didn't explain better. I meant that we don't need a case where we don't want to chain-up, hence we can do: yield base.setup (); if (A && B) { ... }
Review of attachment 218553 [details] [review]: ::: src/winxp-installer.vala @@ +45,3 @@ + yield fuse.mount_media (cancellable); + + FileInfo info; I wanted to make the type explicit, but while ((FileInfo info = enumerator.next_file ()) does not work in vala. @@ -41,0 +41,21 @@ + public override async void setup (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { + FuseIso fuse = new FuseIso (extra_iso); ... 18 more ... Can you be more explicit about which comments? FuseIso iso and the call to base.setup() must be in the same block since base.setup is copying the files from the iso which have been added with add_unattended_file. I can move FuseIso declaration up one block in order to be able to put the yield base.setup (...) outside of the if if you want : FuseIso iso = new FuseIso (extra_iso); if (extra_iso != null) { .... } yield base.setup (vm_name, cancellable);
Review of attachment 218554 [details] [review]: ::: src/winxp-installer.vala @@ -41,2 +47,2 @@ public override async void setup (string vm_name, Cancellable? cancellable) throws GLib.Error { - if (extra_iso != null) { + bool have_viostor = false; Hmm kind of. For winxp files must be copied from the CD to the floppy image, for win7, we can directly use the files from the CD. The fact that we copy these files have the side-effect of checking that they exist. I could indeed add some kind of check for win7 as well. @@ -42,4 +48,4 @@ - if (extra_iso != null) { - FuseIso fuse = new FuseIso (extra_iso); - - yield fuse.mount_media (cancellable); + bool have_viostor = false; + has_viostor_drivers = false; + + try { Yeah this code is a bit ugly, but I'd rather have it handle this kind of errors when it can. Otherwise dropping an invalid iso to .cache/gnome-boxes and no longer thinking about it would result in windows installations no longer working @@ +69,3 @@ + add_unattended_file (new UnattendedRawFile (this, full_path, info.get_name ())); + if (info.get_name () == "viostor.sys") + have_viostor = true; Nope, I want to copy all the files that are in this directory. I'm checking for a specific filename as a sanity check of what we are copying.
Created attachment 218654 [details] [review] Add UnattendedRawFile class
Created attachment 218655 [details] [review] Add an argument to Util::run_in_thread() This makes it possible to pass an argument to the function to be run in a separate thread. This is needed to avoid a vala bug (#677261) which was causing the generated code to take a reference on the being-finalized object when trying to use run_in_thread from an object finalizer, leading to lots of bad things.
Created attachment 218656 [details] [review] Add hack to workaround broken IOError.from_errno g_io_error_from_errno returns a GIOErrorEnum, and vala doesn't know it should convert it to a GIOError *, which causes compilation issues with: // valac-0.16 --pkg gio-2.0 foo.vala // /home/teuf/jhbuild-boxes/sources/gnome-boxes/foo.vala.c: In function ‘test’: // /home/teuf/jhbuild-boxes/sources/gnome-boxes/foo.vala.c:26:9: erreur: // incompatible types when assigning to type ‘struct GError *’ from type ‘GIOErrorEnum’ // error: cc exited with status 26 // Compilation failed: 1 error(s), 0 warning(s) void test () throws GLib.Error { throw GLib.IOError.from_errno(22); // EINVAL } int main () { try { test (); } catch (GLib.Error e) {} return 0; } This is vala bug 664530. This workaround was suggested by vala maintainers as the bug isn't trivial to fix.
Created attachment 218657 [details] [review] Add IsoExtractor class It's used to mount an iso image and to read some content from it. Boxes will use it to get the extra drivers to copy to the winxp unattended install floppy.
Created attachment 218658 [details] [review] fedora: remove redundant members FedoraInstaller::initrd_path and FedoraInstaller::kernel_path can be easily inferred from initrd_file and kernel_file so no need to keep those around.
Created attachment 218659 [details] [review] Simplify FedoraInstaller::extract_boot_files It was reusing several times the same variables for different purposes, including assigning a temporary value to member variables to override them later on.
Created attachment 218660 [details] [review] Use IsoExtractor in FedoraInstaller This removes some duplicated code between FedoraInstaller and IsoExtractor
Created attachment 218661 [details] [review] Insert extra iso if available This iso will be used during automatic Windows installations. It contains the viostor driver to be able to use virtio at install time and extra drivers (vioserial, ...) and utilities (vdagent) which are added to the install after it's done. This improves VM integration a lot. The name of the iso is win-tools.iso, and it's currently looked up in $XDG_CACHE_HOME/gnome-boxes/ first, and if not found, it's searched in $datadir/gnome-boxes/unattended.
Created attachment 218662 [details] [review] Add InstallerMedia::supports_virtio_disk method This will be useful to be able to automatically use the viostor driver on Windows when we have it available. We already have a check for that, but this commit moves it to its own method that derived classes can override if they want. This will allow Windows unattended installers to override the info from libosinfo when the virtio drivers can be provided at install time.
Created attachment 218663 [details] [review] win7: use drivers from extra iso at install time If the extra driver iso is available when creating a new win7 box, we can use these drivers during installation to be able to use VirtIO through viostor during the installation, which is desirable for better performance. If the ISO is not available, this entry is silently ignored, so it doesn't hurt to have it always there.
Created attachment 218664 [details] [review] winxp: copy drivers from the extra install ISO Boxes optionally supports using an additional ISO image with extra drivers to install to get better integration after the installation. However, Windows XP (contrary to Windows 7) is not able to read drivers directly from the CD but needs them to be copied to the unattended floppy image.
Created attachment 218665 [details] [review] win:use viostor in unattended installs if possible An extra ISO with the additional drivers need to be available, and Boxes needs to be able to locate it.
Review of attachment 218657 [details] [review]: Thats all I could fish for now ::: src/iso-extractor.vala @@ +2,3 @@ + +// Helper class to extract files from an ISO image +private class Boxes.IsoExtractor: GLib.Object { In Boxes we have kept the caps on abbreviations in names, e.g VMCreator, not VmCreator. @@ +27,3 @@ + return; + + run_in_thread.begin (unmount, mount_point.get_path () ); You wrote: "However, this means the final unlink will run in the mainloop thread, which is one of the things I wanted to avoid" why is that? @@ +36,3 @@ + string? mount_path = GLib.DirUtils.mkdtemp (get_user_pkgcache ("fuse-isoXXXXXX")); + if (mount_path == null) + throw (GLib.IOError) new GLib.Error (G_IO_ERROR, g_io_error_from_errno (42), "fail"); '42' (apart from being the meaning of life) is already known here so perhaps you can find the corresponding GIOError code and just use that Error? Than we can drop that hack to work around g_io_from_errno issue? @@ +49,3 @@ + + public async GLib.FileEnumerator enumerate_children (string rel_path) + throws GLib.Error throws fits on the same line and if 'requires' doesn't fit, you want to align it under the 'string'. @@ +51,3 @@ + throws GLib.Error + requires (mounted) + { '{' immediately after the signature on the same line.. @@ +55,3 @@ + string abs_src_path = Path.build_filename (mount_point.get_path (), rel_path); + File dir = File.new_for_path (abs_src_path); + return yield dir.enumerate_children_async (FILE_ATTRIBUTE_STANDARD_NAME, 0); newline before return @@ +61,3 @@ + throws GLib.Error + requires (mounted) + { '{' immediately after the signature on the same line..
Review of attachment 218548 [details] [review]: ::: src/fuse-iso.vala @@ +25,3 @@ + return; + + run_in_thread.begin (unmount_in_thread, mount_point.get_path () ); * why avoid the mainloop thread? * exec is async so you can't really call it sync. You want to at least change that to exec_sync.
Review of attachment 218553 [details] [review]: ::: src/winxp-installer.vala @@ +45,3 @@ + yield fuse.mount_media (cancellable); + + FileInfo info; Perhaps because you need to do an explicit null check and not have 'info' itself as the condition? @@ -41,0 +41,21 @@ + public override async void setup (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { + FuseIso fuse = new FuseIso (extra_iso); ... 18 more ... hmm.. I think I missed the fact that base.setup() needs the ISO mounted in here. If this is the case, I wonder if this will be clear to others reading this code later. Would be nice for this class not to assume what base.setup does with the unattended_files or anything much. Don't have any good suggestion from the top of my head so let me think abit on how this could be done more cleanly..
The last two patches were supposed to be from before you submitted the latest versions but somehow they were stuck at the 'draft' stage.
(In reply to comment #63) > The last two patches were supposed to be from before you submitted the latest > versions but somehow they were stuck at the 'draft' stage. err.. s/patches/comment/
(In reply to comment #61) > Review of attachment 218548 [details] [review]: > > ::: src/fuse-iso.vala > @@ +25,3 @@ > + return; > + > + run_in_thread.begin (unmount_in_thread, mount_point.get_path () ); > > * why avoid the mainloop thread? I don't want to do sync IO from the mainloop.
(In reply to comment #60) > Review of attachment 218657 [details] [review]: > > @@ +49,3 @@ > + > + public async GLib.FileEnumerator enumerate_children (string rel_path) > + throws GLib.Error > > throws fits on the same line and if 'requires' doesn't fit, you want to align > it under the 'string'. > Attempt at making things actually readable... > @@ +51,3 @@ > + throws GLib.Error > + requires (mounted) > + { > > '{' immediately after the signature on the same line.. > ditto... I'll close my eyes and change this...
(In reply to comment #60) > Review of attachment 218657 [details] [review]: > > @@ +36,3 @@ > + string? mount_path = GLib.DirUtils.mkdtemp (get_user_pkgcache > ("fuse-isoXXXXXX")); > + if (mount_path == null) > + throw (GLib.IOError) new GLib.Error (G_IO_ERROR, > g_io_error_from_errno (42), "fail"); > > '42' (apart from being the meaning of life) is already known here so perhaps > you can find the corresponding GIOError code and just use that Error? Than we > can drop that hack to work around g_io_from_errno issue? This is actually a c&p bug, it's supposed to be s/42/errno and s/"fail"/meaningful error message Thanks for noticing ;)
(In reply to comment #62) > hmm.. I think I missed the fact that base.setup() needs the ISO mounted in > here. If this is the case, I wonder if this will be clear to others reading > this code later. Would be nice for this class not to assume what base.setup > does with the unattended_files or anything much. Don't have any good suggestion > from the top of my head so let me think abit on how this could be done more > cleanly.. I could introduce an UnattendedISOFile class which keeps a ref on the ISOExtractor, this would be more flexible even though base.setup() will still need to be called last.
(In reply to comment #62) > Review of attachment 218553 [details] [review]: > > ::: src/winxp-installer.vala > @@ +45,3 @@ > + yield fuse.mount_media (cancellable); > + > + FileInfo info; > > Perhaps because you need to do an explicit null check and not have 'info' > itself as the condition? > Actually declaring variables as part of the loop seems to only be allowed in for() loops, not in while() loops if I read http://www.vala-project.org/doc/vala-draft/statements.html correctly. I've checked that using 'var' instead of 'FileInfo' in the loop isn't working any better.
(In reply to comment #65) > (In reply to comment #61) > > Review of attachment 218548 [details] [review] [details]: > > > > ::: src/fuse-iso.vala > > @@ +25,3 @@ > > + return; > > + > > + run_in_thread.begin (unmount_in_thread, mount_point.get_path () ); > > > > * why avoid the mainloop thread? > > I don't want to do sync IO from the mainloop. Who asked you to do sync IO? yield always defers to main_loop and in exec() we are using g_io_scheduler_push_job that runs the job in another thread.
(In reply to comment #66) > (In reply to comment #60) > > Review of attachment 218657 [details] [review] [details]: > > > > @@ +49,3 @@ > > + > > + public async GLib.FileEnumerator enumerate_children (string rel_path) > > + throws GLib.Error > > > > throws fits on the same line and if 'requires' doesn't fit, you want to align > > it under the 'string'. > > > > Attempt at making things actually readable... If putthing them on a newline makes them more readable for you, I have no objection but do align them to the arguments. > > @@ +51,3 @@ > > + throws GLib.Error > > + requires (mounted) > > + { > > > > '{' immediately after the signature on the same line.. > > > > ditto... > > I'll close my eyes and change this... I quite disagree with you that this adds anything to readability, if it did, I don't see why its not done for if and other kinds of blocks. Anyway, unless you got any compelling rationale I'd recommend you stick to the coding-style already followed.
(In reply to comment #68) > (In reply to comment #62) > > > hmm.. I think I missed the fact that base.setup() needs the ISO mounted in > > here. If this is the case, I wonder if this will be clear to others reading > > this code later. Would be nice for this class not to assume what base.setup > > does with the unattended_files or anything much. Don't have any good suggestion > > from the top of my head so let me think abit on how this could be done more > > cleanly.. > > I could introduce an UnattendedISOFile class which keeps a ref on the > ISOExtractor, this would be more flexible even though base.setup() will still > need to be called last. Well if still have one of the issues I pointed above, I don't see much point of that. Why not just extract the needed files to a temporary location and unmount the media soon afterwards? We can have a bool on UnattendedFile that dictates whether or not the source needs to be deleted its its destructor or not?
(In reply to comment #69) > (In reply to comment #62) > > Review of attachment 218553 [details] [review] [details]: > > > > ::: src/winxp-installer.vala > > @@ +45,3 @@ > > + yield fuse.mount_media (cancellable); > > + > > + FileInfo info; > > > > Perhaps because you need to do an explicit null check and not have 'info' > > itself as the condition? > > > > Actually declaring variables as part of the loop seems to only be allowed in > for() loops, not in while() loops if I read > http://www.vala-project.org/doc/vala-draft/statements.html correctly. I've > checked that using 'var' instead of 'FileInfo' in the loop isn't working any > better. Interesting, OTOH I just noticed that in this case a for loop is more fitting: for (FileInfo info = enumerator.next_file (); info = enumerator.next_file (); info != null) {
(In reply to comment #70) > (In reply to comment #65) > > (In reply to comment #61) > > > Review of attachment 218548 [details] [review] [details] [details]: > > > > > > ::: src/fuse-iso.vala > > > @@ +25,3 @@ > > > + return; > > > + > > > + run_in_thread.begin (unmount_in_thread, mount_point.get_path () ); > > > > > > * why avoid the mainloop thread? > > > > I don't want to do sync IO from the mainloop. > > Who asked you to do sync IO? yield always defers to main_loop and in exec() we > are using g_io_scheduler_push_job that runs the job in another thread. If it wasn't very clear before, patches from bug#679896 should make it crystal clear.
(In reply to comment #74) > (In reply to comment #70) > > (In reply to comment #65) > > > (In reply to comment #61) > > > > Review of attachment 218548 [details] [review] [details] [details] [details]: > > > > > > > > ::: src/fuse-iso.vala > > > > @@ +25,3 @@ > > > > + return; > > > > + > > > > + run_in_thread.begin (unmount_in_thread, mount_point.get_path () ); > > > > > > > > * why avoid the mainloop thread? > > > > > > I don't want to do sync IO from the mainloop. > > > > Who asked you to do sync IO? yield always defers to main_loop and in exec() we > > are using g_io_scheduler_push_job that runs the job in another thread. > > If it wasn't very clear before, patches from bug#679896 should make it crystal > clear. Can you _read_ my answers before adding 2 comments missing the point by being about exec? This is irritating... Comment #40 says: "Hmm, all of this is very interesting, I hadn't thought of things this way. However, this means the final unlink will run in the mainloop thread, which is one of the things I wanted to avoid, so I'd favour keeping the current code." The unmount() code is void unmount() { exec_sync("umount"); unlink(); }, I agree exec can be easily done async, but then the unlink() ends up being run in an idle, ie in the main thread as far as I understand
(In reply to comment #73) > Interesting, OTOH I just noticed that in this case a for loop is more fitting: > > for (FileInfo info = enumerator.next_file (); > info = enumerator.next_file (); > info != null) { I disagree this is more fitting, calling enumerator.next_file (); twice is weird, I prefer to keep the while() loop. That said, I really should be using enumerator.enumerate_children_async there, which will avoid this issue.
(In reply to comment #71) > (In reply to comment #66) > > (In reply to comment #60) > > > Review of attachment 218657 [details] [review] [details] [details]: > > > > > > @@ +49,3 @@ > > > + > > > + public async GLib.FileEnumerator enumerate_children (string rel_path) > > > + throws GLib.Error > > > > > > throws fits on the same line and if 'requires' doesn't fit, you want to align > > > it under the 'string'. > > > > > > > Attempt at making things actually readable... > > If putthing them on a newline makes them more readable for you, I have no > objection but do align them to the arguments. > throws/requires is totally unrelated to the function arguments, so aligning them is not really logical, and moving them to the far right of the screen makes them harder to spot/read. > > > @@ +51,3 @@ > > > + throws GLib.Error > > > + requires (mounted) > > > + { > > > > > > '{' immediately after the signature on the same line.. > > > > > > > ditto... > > > > I'll close my eyes and change this... > > I quite disagree with you that this adds anything to readability, if it did, I > don't see why its not done for if and other kinds of blocks. Anyway, unless you > got any compelling rationale I'd recommend you stick to the coding-style > already followed. In this case we've got 2 weird blocks floating around (throws/requires) and a multiline function definition, which makes the { ending up in a quite random/arbitrary place. I've changed them to what you want.
(In reply to comment #72) > (In reply to comment #68) > > (In reply to comment #62) > > > > > hmm.. I think I missed the fact that base.setup() needs the ISO mounted in > > > here. If this is the case, I wonder if this will be clear to others reading > > > this code later. Would be nice for this class not to assume what base.setup > > > does with the unattended_files or anything much. Don't have any good suggestion > > > from the top of my head so let me think abit on how this could be done more > > > cleanly.. > > > > I could introduce an UnattendedISOFile class which keeps a ref on the > > ISOExtractor, this would be more flexible even though base.setup() will still > > need to be called last. > > Well if still have one of the issues I pointed above, I don't see much point of > that. > > Why not just extract the needed files to a temporary location and unmount the > media soon afterwards? We can have a bool on UnattendedFile that dictates > whether or not the source needs to be deleted its its destructor or not? I think I don't understand what your issues are. With your suggestion, base.setup() will still need to be called after the various fedora-specific UnattendedFile instances have been created and add registered with the UnattendedInstaller instance. However, I don't think it's wrong to have this expectation that base.setup() is called at the right time, this should be seen as just a helper function which avoids duplicating code in FedoraInstaller::setup. With my UnattendedISOFile suggestion, the iso will be mounted as long as needed, and will be automatically unmounted when the last UnattendedISOFile instance goes away, which is much nicer than what is currently done in this patch.
(In reply to comment #78) > (In reply to comment #72) > > (In reply to comment #68) > > > (In reply to comment #62) > > > > > > > hmm.. I think I missed the fact that base.setup() needs the ISO mounted in > > > > here. If this is the case, I wonder if this will be clear to others reading > > > > this code later. Would be nice for this class not to assume what base.setup > > > > does with the unattended_files or anything much. Don't have any good suggestion > > > > from the top of my head so let me think abit on how this could be done more > > > > cleanly.. > > > > > > I could introduce an UnattendedISOFile class which keeps a ref on the > > > ISOExtractor, this would be more flexible even though base.setup() will still > > > need to be called last. > > > > Well if still have one of the issues I pointed above, I don't see much point of > > that. > > > > Why not just extract the needed files to a temporary location and unmount the > > media soon afterwards? We can have a bool on UnattendedFile that dictates > > whether or not the source needs to be deleted its its destructor or not? > > I think I don't understand what your issues are. With your suggestion, > base.setup() will still need to be called after the various fedora-specific > UnattendedFile instances have been created and add registered with the > UnattendedInstaller instance. However, I don't think it's wrong to have this > expectation that base.setup() is called at the right time, How does base class know what is the right time? By knowing the details of what base.setup exactly does. IMO that goes against the idea of 'information hiding'. > With my UnattendedISOFile suggestion, the iso will be mounted as long as > needed, and will be automatically unmounted when the last UnattendedISOFile > instance goes away, which is much nicer than what is currently done in this > patch. I'm not really against that idea, just wanted to avoid yet another class. :) Go for it!
(In reply to comment #79) > (In reply to comment #78) > > I think I don't understand what your issues are. With your suggestion, > > base.setup() will still need to be called after the various fedora-specific > > UnattendedFile instances have been created and add registered with the > > UnattendedInstaller instance. However, I don't think it's wrong to have this > > expectation that base.setup() is called at the right time, > > How does base class know what is the right time? By knowing the details of what > base.setup exactly does. IMO that goes against the idea of 'information > hiding'. Derived class writers have to know what the base class method they are overriding is doing to decide if they want to call it or not, so there is not much information hiding at play here, on the contrary. Information hiding is not about hiding what a method is doing, it's about hiding how it's doing it.
(In reply to comment #79) > > With my UnattendedISOFile suggestion, the iso will be mounted as long as > > needed, and will be automatically unmounted when the last UnattendedISOFile > > instance goes away, which is much nicer than what is currently done in this > > patch. > > I'm not really against that idea, just wanted to avoid yet another class. :) Go > for it! I just realized that there is no need for "yet another class", I ended up doing s/UnattendedRawFile/UnattendedISOFile
Created attachment 219099 [details] [review] Add hack to workaround broken IOError.from_errno g_io_error_from_errno returns a GIOErrorEnum, and vala doesn't know it should convert it to a GIOError *, which causes compilation issues with: // valac-0.16 --pkg gio-2.0 foo.vala // /home/teuf/jhbuild-boxes/sources/gnome-boxes/foo.vala.c: In function ‘test’: // /home/teuf/jhbuild-boxes/sources/gnome-boxes/foo.vala.c:26:9: erreur: // incompatible types when assigning to type ‘struct GError *’ from type ‘GIOErrorEnum’ // error: cc exited with status 26 // Compilation failed: 1 error(s), 0 warning(s) void test () throws GLib.Error { throw GLib.IOError.from_errno(22); // EINVAL } int main () { try { test (); } catch (GLib.Error e) {} return 0; } This is vala bug 664530. This workaround was suggested by vala maintainers as the bug isn't trivial to fix.
Created attachment 219100 [details] [review] Add IsoExtractor class It's used to mount an iso image and to read some content from it. Boxes will use it to get the extra drivers to copy to the winxp unattended install floppy.
Created attachment 219101 [details] [review] Add UnattendedISOFile class
Created attachment 219102 [details] [review] fedora: remove redundant members FedoraInstaller::initrd_path and FedoraInstaller::kernel_path can be easily inferred from initrd_file and kernel_file so no need to keep those around.
Created attachment 219103 [details] [review] fedora: simplify extract_boot_files It was reusing several times the same variables for different purposes, including assigning a temporary value to member variables to override them later on.
Created attachment 219105 [details] [review] fedora: use IsoExtractor This removes some duplicated code between FedoraInstaller and IsoExtractor
Created attachment 219106 [details] [review] installer: add helper to add iso image to a domain This helper is only used in InstallerMedia::setup_domain_config for now, but it will be useful to insert extra ISO images needed during unattended installations.
Created attachment 219107 [details] [review] installer: insert extra iso if available This iso will be used during automatic Windows installations. It contains the viostor driver to be able to use virtio at install time and extra drivers (vioserial, ...) and utilities (vdagent) which are added to the install after it's done. This improves VM integration a lot. The name of the iso is win-tools.iso, and it's currently looked up in $XDG_CACHE_HOME/gnome-boxes/ first, and if not found, it's searched in $datadir/gnome-boxes/unattended.
Created attachment 219108 [details] [review] installer: add ::supports_virtio_disk method This will be useful to be able to automatically use the viostor driver on Windows when we have it available. We already have a check for that, but this commit moves it to its own method that derived classes can override if they want. This will allow Windows unattended installers to override the info from libosinfo when the virtio drivers can be provided at install time.
Created attachment 219109 [details] [review] win7: use drivers from extra iso at install time If the extra driver iso is available when creating a new win7 box, we can use these drivers during installation to be able to use VirtIO through viostor during the installation, which is desirable for better performance. If the ISO is not available, this entry is silently ignored, so it doesn't hurt to have it always there.
Created attachment 219111 [details] [review] winxp: copy drivers from the extra install ISO Boxes optionally supports using an additional ISO image with extra drivers to install to get better integration after the installation. However, Windows XP (contrary to Windows 7) is not able to read drivers directly from the CD but needs them to be copied to the unattended floppy image.
Created attachment 219112 [details] [review] win:use viostor in unattended installs if possible An extra ISO with the additional drivers need to be available, and Boxes needs to be able to locate it.
Review of attachment 218548 [details] [review]: ::: src/fuse-iso.vala @@ +25,3 @@ + return; + + run_in_thread.begin (unmount_in_thread, mount_point.get_path () ); Good news is that I just found out the '-p' option to fuseiso: -p -- maintain mount point; create it if it does not exists and delete it on exit This removes the need for the unlink call in the destructor, and will thus make this code simpler.
Created attachment 219495 [details] [review] Add ISOExtractor class It's used to mount an iso image and to read some content from it. Boxes will use it to get the extra drivers to copy to the winxp unattended install floppy.
Review of attachment 219099 [details] [review]: config.vapi is meant for build configuration parameters so this doesn't at all belong there. We should have it in util.vala or if it needs to be in a vapi file for some reason, feel free to add a 'hacks.vapi' file. ::: src/config.vapi @@ -9,2 +9,4 @@ public const string DATADIR; } + +// hack because GLib.IOError.from_errno and GLib.g_io_error_from_errno * There is no such thing as 'GLib.IOError.from_errno' AFAICT. * Capitalize 'h' please? :)
Review of attachment 219101 [details] [review]: Apart from these nitpicks, the only problem I see is that this class does really nothing different than its siblings than to keep a ref to it extractor. Something that could be achieved with GLib.Object.weak_ref ? ::: src/unattended-installer.vala @@ -377,0 +377,13 @@ +private class Boxes.UnattendedISOFile : GLib.Object, Boxes.UnattendedFile { + protected string src_path { get; set; } + protected string dest_name { get; set; } ... 10 more ... * The part before ',' should be on top of the class definition itself and rest should be on top of the 'extractor' field declaration, IMO. * Capitalize 't' please * lines are 120 character * Use '//' for comments
Review of attachment 219102 [details] [review]: ACK
Review of attachment 219103 [details] [review]: Looks good
Review of attachment 219105 [details] [review]: Looks good otherwise. ::: src/fedora-installer.vala @@ -70,1 +63,2 @@ - yield mount_media (cancellable); + if (express_toggle.active) + if (os_media.kernel_path != null && os_media.initrd_path != null) { I'd prefer this part to remain as it was. I think its more readable. Talking of the two 'if' blocks that simply return. @@ +63,3 @@ + if (express_toggle.active) + if (os_media.kernel_path != null && os_media.initrd_path != null) { + ISOExtractor extractor = new ISOExtractor (device_file); In this case you must admit 'var' is better? :)
Review of attachment 219106 [details] [review]: ::: src/installer-media.vala @@ +69,2 @@ public virtual void setup_domain_config (Domain domain) { + add_cd_drive(domain, from_image?DomainDiskType.FILE:DomainDiskType.BLOCK, device_file, "hdc", true); Missing some spaces on this line. @@ -106,2 +91,3 @@ } + protected void add_cd_drive(Domain domain, DomainDiskType type, string iso_path, string device_name, bool mandatory = false) * I'd name it 'add_cdrom_config' to be clear. * Space missing here too.
Review of attachment 219107 [details] [review]: Looks good otherwise. ::: src/unattended-installer.vala @@ -153,2 +159,4 @@ } + public void add_extra_iso (Domain domain) { + /* Allows us to add an extra install media, for example with more * Use '//' for comments * line is 120 chars * This comment should IMO be above method not under it. @@ -155,0 +161,8 @@ + public void add_extra_iso (Domain domain) { + /* Allows us to add an extra install media, for example with more + * windows drivers ... 5 more ... Space before '('. ::: src/win7-installer.vala @@ -47,1 +49,5 @@ os.name); + extra_iso = "win-tools.iso"; + } + + public override void setup_domain_config (Domain domain) { so why is this in win7 and winxp classes and not just done in unattended-installer if rest is already there?
Review of attachment 219108 [details] [review]: Looks good otherwise. ::: src/vm-configurator.vala @@ +184,3 @@ + + if (install_media.supports_virtio_disk ()) { + debug ("Using virtio controller for the install disk"); 'install disk' doesn't sound correct to me. How about 'main storage volume' or just 'main disk'?
Review of attachment 219109 [details] [review]: ACK assuming you tested this (judging from commit log).
Review of attachment 219111 [details] [review]: Like the rest, looks good otherwise. :) ::: src/winxp-installer.vala @@ -45,2 +45,4 @@ } + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { IMHO the lesser the indentation, the more readable the code. So I'd do: if (extra_iso == null) return; // Rest follows here @@ -47,0 +47,7 @@ + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { + ISOExtractor extractor = new ISOExtractor (extra_iso); ... 4 more ... We've been prefering use of '?' in such cases. I didn't like that myself but now I agree with elmarco that its more readable. Since its vary clear that a string is being assigned, we should use 'var': var driver_path = (os_media.architecture == "x86_64") ? "preinst/winxp/amd64" : "preinst/winxp/x86"; @@ -47,0 +47,14 @@ + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { + ISOExtractor extractor = new ISOExtractor (extra_iso); ... 11 more ... I still think for is more appropriate and readable here and it doesn't matter if initialization and increment statements are exactly the same. You can keep the initialization statement before 'for' if that makes any diff to you: GLib.List<FileInfo> infos = infos = yield enumerator.next_files_async (4, GLib.Priority.DEFAULT, cancellable); for (; infos != null; infos = yield enumerator.next_files_async (4, GLib.Priority.DEFAULT, cancellable)) { @@ +63,3 @@ + string relative_path = Path.build_filename (driver_path, info.get_name ()); + string full_path = extractor.get_absolute_path (relative_path); + debug ("copying %s from extra ISO to unattended floppy", relative_path); Capital 'C'
Review of attachment 219112 [details] [review]: Looking good. commit log: 'win:use' -> 'win: Use' ::: src/winxp-installer.vala @@ +47,3 @@ } + public override bool supports_virtio_disk () { Now that I see it being overriden here, this could be a (virtual) property instead? @@ +82,3 @@ } + yield base.prepare_for_installation (vm_name, cancellable); + // if we arrived there with no exception, then everything is setup for if -> If @@ +85,3 @@ + // the Windows installer to be able to use a virtio disk controller + has_viostor_drivers = have_viostor; + } catch (GLib.Error e) { Why we ignores all errors and why we didn't before?
Review of attachment 219495 [details] [review]: Looks good otherwise ::: src/iso-extractor.vala @@ +25,3 @@ + return; + + string temp_name = get_user_pkgcache ("fuse-isoXXXXXX"); XXXXXX ? :) @@ +28,3 @@ + string? mount_point = GLib.DirUtils.mkdtemp (temp_name); + if (mount_point == null) + throw (GLib.IOError) new GLib.Error (G_IO_ERROR, g_io_error_from_errno (errno), "failed to mount %s", temp_name); failed -> Failed
(In reply to comment #96) > Review of attachment 219099 [details] [review]: > > config.vapi is meant for build configuration parameters so this doesn't at all > belong there. We should have it in util.vala or if it needs to be in a vapi > file for some reason, feel free to add a 'hacks.vapi' file. No clue how to move this to a .vala file: util.vala:14.1-14.28: error: A const field requires a value to be provided public const int G_IO_ERROR; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ util.vala:17.1-17.32: error: Non-abstract, non-extern methods must have bodies public int g_io_error_from_errno (int err_no); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Compilation failed: 2 error(s), 0 warning(s)
(In reply to comment #97) > Review of attachment 219101 [details] [review]: > > Apart from these nitpicks, the only problem I see is that this class does > really nothing different than its siblings than to keep a ref to it extractor. > Something that could be achieved with GLib.Object.weak_ref ? > How would that work? The point of weak_ref is to avoid taking a ref on the object (ie not keep it alive), but to get notified when the object goes away instead.
Review of attachment 219107 [details] [review]: ::: src/win7-installer.vala @@ -47,1 +49,5 @@ os.name); + extra_iso = "win-tools.iso"; + } + + public override void setup_domain_config (Domain domain) { This could go in win-installer.vala I guess
Review of attachment 219111 [details] [review]: ::: src/winxp-installer.vala @@ -45,2 +45,4 @@ } + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { We've got to call yield base.prepare_for_installation (vm_name, cancellable); in the !extra_iso case as well. @@ -47,0 +47,7 @@ + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { + ISOExtractor extractor = new ISOExtractor (extra_iso); ... 4 more ... This makes for a far too long line to be easy to parse... Will change. @@ -47,0 +47,14 @@ + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { + ISOExtractor extractor = new ISOExtractor (extra_iso); ... 11 more ... And I still disagree, and splinter is not helping by not letting me quote the relevant part and cut the rest. @@ +63,3 @@ + string relative_path = Path.build_filename (driver_path, info.get_name ()); + string full_path = extractor.get_absolute_path (relative_path); + debug ("copying %s from extra ISO to unattended floppy", relative_path); Half of the debug() messages start with a lowercase.... Changed.
Review of attachment 219112 [details] [review]: ::: src/winxp-installer.vala @@ +47,3 @@ } + public override bool supports_virtio_disk () { No idea how this works. @@ +85,3 @@ + // the Windows installer to be able to use a virtio disk controller + has_viostor_drivers = have_viostor; + } catch (GLib.Error e) { I want errors happening during anything involving the drivers iso to be non-fatal and just disable use of these drivers. However, not all errors from yield base.prepare_for_installation should ignored. Let's see how to fix that...
Review of attachment 219495 [details] [review]: ::: src/iso-extractor.vala @@ +25,3 @@ + return; + + string temp_name = get_user_pkgcache ("fuse-isoXXXXXX"); yes XXXXXX, it's passed to GLib.DirUtils.mkdtemp on the next line.
(In reply to comment #110) > (In reply to comment #96) > > Review of attachment 219099 [details] [review] [details]: > > > > config.vapi is meant for build configuration parameters so this doesn't at all > > belong there. We should have it in util.vala or if it needs to be in a vapi > > file for some reason, feel free to add a 'hacks.vapi' file. > > No clue how to move this to a .vala file: > util.vala:14.1-14.28: error: A const field requires a value to be provided > public const int G_IO_ERROR; > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > util.vala:17.1-17.32: error: Non-abstract, non-extern methods must have bodies > public int g_io_error_from_errno (int err_no); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Compilation failed: 2 error(s), 0 warning(s) It might not be possible to override bindings from Vala file but please do ask on vala irc/list and its the case, we can go with that separate vapi approach I mentioned. Just that this doesn't belong in this vapi at all.
(In reply to comment #111) > (In reply to comment #97) > > Review of attachment 219101 [details] [review] [details]: > > > > Apart from these nitpicks, the only problem I see is that this class does > > really nothing different than its siblings than to keep a ref to it extractor. > > Something that could be achieved with GLib.Object.weak_ref ? > > > > How would that work? The point of weak_ref is to avoid taking a ref on the > object (ie not keep it alive), but to get notified when the object goes away > instead. Yes but we don't want to add ref to the target of weak_ref (UnattendedFile) but create a ref to ISOExtractor until UnattendedFile is finalized: unattended_file.weak_ref (() => { /* Do something to ISOExtractor (e.g manually unmount or set the local var holding ?ISOExtractor to null?) */ }); We can also make use of GObject.set_data here or have an optional GObject field in UnattendedFile that can be used to keep reference to and arbitrary object around. Just that having a class with plenty of redundant code seems wrong to me.
(In reply to comment #117) > (In reply to comment #111) > > (In reply to comment #97) > > > Review of attachment 219101 [details] [review] [details] [details]: > > > > > > Apart from these nitpicks, the only problem I see is that this class does > > > really nothing different than its siblings than to keep a ref to it extractor. > > > Something that could be achieved with GLib.Object.weak_ref ? > > > > > > > How would that work? The point of weak_ref is to avoid taking a ref on the > > object (ie not keep it alive), but to get notified when the object goes away > > instead. > > Yes but we don't want to add ref to the target of weak_ref (UnattendedFile) but > create a ref to ISOExtractor until UnattendedFile is finalized: > > unattended_file.weak_ref (() => { /* Do something to ISOExtractor (e.g manually > unmount or set the local var holding ?ISOExtractor to null?) */ }); > > We can also make use of GObject.set_data here or have an optional GObject field > in UnattendedFile that can be used to keep reference to and arbitrary object > around. Just that having a class with plenty of redundant code seems wrong to > me. So we are using a language where implementing a derived class is easy, but you are suggesting using all sorts of workarounds rather than adding a new class?
Review of attachment 219111 [details] [review]: ::: src/winxp-installer.vala @@ -45,2 +45,4 @@ } + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { Ah yes so there will be one more statement in 'if' block then but I still think its better to keep the bigger block (the main part of this method) outside the 'if'.. @@ -47,0 +47,7 @@ + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { + ISOExtractor extractor = new ISOExtractor (extra_iso); ... 4 more ... You can always split the line if its way too long. :) @@ +63,3 @@ + string relative_path = Path.build_filename (driver_path, info.get_name ()); + string full_path = extractor.get_absolute_path (relative_path); + debug ("copying %s from extra ISO to unattended floppy", relative_path); Yeah, elmarco does that and I get tired of pointing it all the time and too lazy to fix those.. :)
Review of attachment 219112 [details] [review]: ::: src/winxp-installer.vala @@ +47,3 @@ } + public override bool supports_virtio_disk () { Read, try and/or ask before replying to my question then? :)
Review of attachment 219111 [details] [review]: ::: src/winxp-installer.vala @@ -45,2 +45,4 @@ } + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { This duplicates code, but if this is what you want...
Review of attachment 219495 [details] [review]: ::: src/iso-extractor.vala @@ +25,3 @@ + return; + + string temp_name = get_user_pkgcache ("fuse-isoXXXXXX"); Ah ok, just needed to read mkdtemp manpage. :) I think preifx should be the name of the ISO file being mounted. The reason is that in the debug log, it'll help us know which ISO the message(s) is about.
Review of attachment 219111 [details] [review]: ::: src/winxp-installer.vala @@ -45,2 +45,4 @@ } + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { It does?
(In reply to comment #118) > (In reply to comment #117) > > (In reply to comment #111) > > > (In reply to comment #97) > > > > Review of attachment 219101 [details] [review] [details] [details] [details]: > > > > > > > > Apart from these nitpicks, the only problem I see is that this class does > > > > really nothing different than its siblings than to keep a ref to it extractor. > > > > Something that could be achieved with GLib.Object.weak_ref ? > > > > > > > > > > How would that work? The point of weak_ref is to avoid taking a ref on the > > > object (ie not keep it alive), but to get notified when the object goes away > > > instead. > > > > Yes but we don't want to add ref to the target of weak_ref (UnattendedFile) but > > create a ref to ISOExtractor until UnattendedFile is finalized: > > > > unattended_file.weak_ref (() => { /* Do something to ISOExtractor (e.g manually > > unmount or set the local var holding ?ISOExtractor to null?) */ }); > > > > We can also make use of GObject.set_data here or have an optional GObject field > > in UnattendedFile that can be used to keep reference to and arbitrary object > > around. Just that having a class with plenty of redundant code seems wrong to > > me. > > So we are using a language where implementing a derived class is easy, but you > are suggesting using all sorts of workarounds rather than adding a new class? You have already written the class so I don't need to save any efforts there. :) I just dont think having a separate class just to keep a ref is justified here were you have to have >10 redundant LOC. Also not everything i mentioned above is a work-around.
Review of attachment 219111 [details] [review]: ::: src/winxp-installer.vala @@ -45,2 +45,4 @@ } + public override async void prepare_for_installation (string vm_name, Cancellable? cancellable) throws GLib.Error { + if (extra_iso != null) { if (extra_iso == null) { yield extractor.mount_media (cancellable); return; } .... yield extractor.mount_media (cancellable);
(In reply to comment #124) > I just dont think having a separate class just to keep a ref is justified > here were you have to have >10 redundant LOC. Are you talking about the various properties and their initialisation in the constructor that have to be there because you insist on UnattendedFile to be an interface rather than a proper base class? I'm all for changing that...
Created attachment 220524 [details] [review] Add workaround for broken IOError.from_errno binding g_io_error_from_errno returns a GIOErrorEnum, and vala doesn't know it should convert it to a GIOError *, which causes compilation issues with: // valac-0.16 --pkg gio-2.0 foo.vala // /home/teuf/jhbuild-boxes/sources/gnome-boxes/foo.vala.c: In function ‘test’: // /home/teuf/jhbuild-boxes/sources/gnome-boxes/foo.vala.c:26:9: erreur: // incompatible types when assigning to type ‘struct GError *’ from type ‘GIOErrorEnum’ // error: cc exited with status 26 // Compilation failed: 1 error(s), 0 warning(s) void test () throws GLib.Error { throw GLib.IOError.from_errno(22); // EINVAL } int main () { try { test (); } catch (GLib.Error e) {} return 0; } This is vala bug 664530. This workaround was suggested by vala maintainers as the bug isn't trivial to fix.
Created attachment 220525 [details] [review] Add ISOExtractor class It's used to mount an iso image and to read some content from it. Boxes will use it to get the extra drivers to copy to the winxp unattended install floppy.
Created attachment 220526 [details] [review] Add UnattendedISOFile class
Created attachment 220527 [details] [review] fedora: Remove redundant members FedoraInstaller::initrd_path and FedoraInstaller::kernel_path can be easily inferred from initrd_file and kernel_file so no need to keep those around.
Created attachment 220528 [details] [review] fedora: Simplify extract_boot_files It was reusing several times the same variables for different purposes, including assigning a temporary value to member variables to override them later on.
Created attachment 220529 [details] [review] fedora: Use ISOExtractor This removes some duplicated code between FedoraInstaller and ISOExtractor
Created attachment 220530 [details] [review] installer: Add helper to add iso image to a domain This helper is only used in InstallerMedia::setup_domain_config for now, but it will be useful to insert extra ISO images needed during unattended installations.
Created attachment 220531 [details] [review] installer: Insert extra iso if available This iso will be used during automatic Windows installations. It contains the viostor driver to be able to use virtio at install time and extra drivers (vioserial, ...) and utilities (vdagent) which are added to the install after it's done. This improves VM integration a lot. The name of the iso is win-tools.iso, and it's currently looked up in $XDG_CACHE_HOME/gnome-boxes/ first, and if not found, it's searched in $datadir/gnome-boxes/unattended.
Created attachment 220532 [details] [review] installer: Add ::supports_virtio_disk method This will be useful to be able to automatically use the viostor driver on Windows when we have it available. We already have a check for that, but this commit moves it to its own method that derived classes can override if they want. This will allow Windows unattended installers to override the info from libosinfo when the virtio drivers can be provided at install time.
Created attachment 220533 [details] [review] win7: Use drivers from extra iso at install time If the extra driver iso is available when creating a new win7 box, we can use these drivers during installation to be able to use VirtIO through viostor during the installation, which is desirable for better performance. If the ISO is not available, this entry is silently ignored, so it doesn't hurt to have it always there.
Created attachment 220534 [details] [review] winxp: Copy drivers from the extra install ISO Boxes optionally supports using an additional ISO image with extra drivers to install to get better integration after the installation. However, Windows XP (contrary to Windows 7) is not able to read drivers directly from the CD but needs them to be copied to the unattended floppy image.
Created attachment 220535 [details] [review] win: Use viostor in unattended installs if possible An extra ISO with the additional drivers need to be available, and Boxes needs to be able to locate it.
Created attachment 220536 [details] [review] win7: Run postinst commands from extra iso during install If the extra driver iso is available when creating a new win7 box, we can run the post-install commands it provides. This is useful to automatically install all needed drivers (QXL, vioserial) as well as extra tools for better integration (vdagent). If the ISO is not available, this entry is silently ignored, so it doesn't hurt to have it always there.
Created attachment 220537 [details] [review] winxp: Support post-install commands This is achieved by adding a [GuiRunOnce] section to winxp.sif. When no post-install command is provided, this section will be empty, which should be fine.
Comment on attachment 220536 [details] [review] win7: Run postinst commands from extra iso during install Attached by mistake
Comment on attachment 220537 [details] [review] winxp: Support post-install commands attached by mistake
Comment on attachment 220527 [details] [review] fedora: Remove redundant members This was accepted_commit_now, dunno if I'm supposed to set the state back to that.
Comment on attachment 220528 [details] [review] fedora: Simplify extract_boot_files This was accepted_commit_now, dunno if I'm supposed to set the state back to that.
Comment on attachment 220533 [details] [review] win7: Use drivers from extra iso at install time This was accepted_commit_now, dunno if I'm supposed to set the state back to that.
This new series should address the review comments except for one comment which I cannot find easily now. This was the one about ignoring exceptions during unattended file copies which I said I'd have to think about (ie one more iteration of this specific patch will be needed).
(In reply to comment #146) > This new series should address the review comments except for one comment which > I cannot find easily now. This was the one about ignoring exceptions during > unattended file copies which I said I'd have to think about (ie one more > iteration of this specific patch will be needed). Its OK, it wasn't really a big issue IIRC. So I'll simply ACK your patches trusting that they are the same but with the issues I pointed out addressed.
Created attachment 221180 [details] [review] Workaround race condition in Util::exec The C code generated by vala for Util::exec is racy because it does not make a copy of argv for its own use. Since argv is then used in a run_in_thread callback, it may have been freed between the time Util::exec returned and the time the callback was run in the thread. 18:18 <@juergbi> teuf: arrays are not implicitly copied as arrays are not reference counted and this breaks at least one use case (byte buffer passed to async read()) 18:18 <@juergbi> either the caller has to make sure that the array stays alive until the end of the async function call 18:19 <@juergbi> or you should be explicit that you want to keep the array stored as part of the async function 18:19 <@juergbi> you can do that by marking the argv parameter as 'owned' 18:19 <@juergbi> (or copying it into a local variable) Using 'owned' does not work as expected (I triggered an argv double-free in the generated code), so let's use a local variable for now, this forces a copy of the argv array and works as expected.
Review of attachment 221180 [details] [review]: thanks for digging this, looks good to me
Created attachment 221393 [details] [review] Add workaround for broken IOError.from_errno binding g_io_error_from_errno returns a GIOErrorEnum, and vala doesn't know it should convert it to a GIOError *, which causes compilation issues with: // valac-0.16 --pkg gio-2.0 foo.vala // /home/teuf/jhbuild-boxes/sources/gnome-boxes/foo.vala.c: In function ‘test’: // /home/teuf/jhbuild-boxes/sources/gnome-boxes/foo.vala.c:26:9: erreur: // incompatible types when assigning to type ‘struct GError *’ from type ‘GIOErrorEnum’ // error: cc exited with status 26 // Compilation failed: 1 error(s), 0 warning(s) void test () throws GLib.Error { throw GLib.IOError.from_errno(22); // EINVAL } int main () { try { test (); } catch (GLib.Error e) {} return 0; } This is vala bug 664530. This workaround was suggested by vala maintainers as the bug isn't trivial to fix.
Created attachment 221394 [details] [review] Workaround race condition in Util::exec The C code generated by vala for Util::exec is racy because it does not make a copy of argv for its own use. Since argv is then used in a run_in_thread callback, it may have been freed between the time Util::exec returned and the time the callback was run in the thread. 18:18 <@juergbi> teuf: arrays are not implicitly copied as arrays are not reference counted and this breaks at least one use case (byte buffer passed to async read()) 18:18 <@juergbi> either the caller has to make sure that the array stays alive until the end of the async function call 18:19 <@juergbi> or you should be explicit that you want to keep the array stored as part of the async function 18:19 <@juergbi> you can do that by marking the argv parameter as 'owned' 18:19 <@juergbi> (or copying it into a local variable) Using 'owned' does not work as expected (I triggered an argv double-free in the generated code), so let's use a local variable for now, this forces a copy of the argv array and works as expected.
Created attachment 221395 [details] [review] Add ISOExtractor class It's used to mount an iso image and to read some content from it. Boxes will use it to get the extra drivers to copy to the winxp unattended install floppy.
Created attachment 221396 [details] [review] Add UnattendedRawFile class
Created attachment 221397 [details] [review] fedora: Remove redundant members FedoraInstaller::initrd_path and FedoraInstaller::kernel_path can be easily inferred from initrd_file and kernel_file so no need to keep those around.
Created attachment 221398 [details] [review] fedora: Simplify extract_boot_files It was reusing several times the same variables for different purposes, including assigning a temporary value to member variables to override them later on.
Created attachment 221399 [details] [review] fedora: Use ISOExtractor This removes some duplicated code between FedoraInstaller and ISOExtractor
Created attachment 221400 [details] [review] installer: Add helper to add iso image to a domain This helper is only used in InstallerMedia::setup_domain_config for now, but it will be useful to insert extra ISO images needed during unattended installations.
Created attachment 221401 [details] [review] installer: Insert extra iso if available This iso will be used during automatic Windows installations. It contains the viostor driver to be able to use virtio at install time and extra drivers (vioserial, ...) and utilities (vdagent) which are added to the install after it's done. This improves VM integration a lot. The name of the iso is win-tools.iso, and it's currently looked up in $XDG_CACHE_HOME/gnome-boxes/ first, and if not found, it's searched in $datadir/gnome-boxes/unattended.
Created attachment 221402 [details] [review] installer: Add ::supports_virtio_disk method This will be useful to be able to automatically use the viostor driver on Windows when we have it available. We already have a check for that, but this commit moves it to its own method that derived classes can override if they want. This will allow Windows unattended installers to override the info from libosinfo when the virtio drivers can be provided at install time.
Created attachment 221403 [details] [review] win7: Use drivers from extra iso at install time If the extra driver iso is available when creating a new win7 box, we can use these drivers during installation to be able to use VirtIO through viostor during the installation, which is desirable for better performance. If the ISO is not available, this entry is silently ignored, so it doesn't hurt to have it always there.
Created attachment 221404 [details] [review] winxp: Copy drivers from the extra install ISO Boxes optionally supports using an additional ISO image with extra drivers to install to get better integration after the installation. However, Windows XP (contrary to Windows 7) is not able to read drivers directly from the CD but needs them to be copied to the unattended floppy image.
Created attachment 221405 [details] [review] win: Use viostor in unattended installs if possible An extra ISO with the additional drivers need to be available, and Boxes needs to be able to locate it.
So... This was supposed to be the final posting of this series, it has been tested and everything. The only changes were in "Add UnattendedRawFile class" and "winxp: Copy drivers from the extra install ISO", but when looking at them, I realized they are not the correct patches... Need to remake them and test again :(
Ah, I think it's good actually, except that the changes are in "win: Use viostor in unattended installs if possible" instead of "winxp: Copy drivers from the extra install ISO", I'll fix that
Created attachment 221407 [details] [review] winxp: Copy drivers from the extra install ISO Boxes optionally supports using an additional ISO image with extra drivers to install to get better integration after the installation. However, Windows XP (contrary to Windows 7) is not able to read drivers directly from the CD but needs them to be copied to the unattended floppy image.
Created attachment 221408 [details] [review] win: Use viostor in unattended installs if possible An extra ISO with the additional drivers need to be available, and Boxes needs to be able to locate it.
It seems it didn't make it in the 3.5.90 release, what shall we do now with this series? Ask for a freeze break? Or punt to 3.7?
(In reply to comment #167) > It seems it didn't make it in the 3.5.90 release, what shall we do now with > this series? Ask for a freeze break? Or punt to 3.7? I'd ask for freeze break. AFAIK feature freeze is mostly at UI-level stuff that will affect testing. Note that we are also in string announcement period. I was worried about something more difficult problem w.r.t these patches: These patches conflict with the 'libosinfo based installer' (bug#676537) and it wont be trivial to rebase either these patches on top of that or vice versa. Please tell me you have a plan about that, cause I don't.
I was under the impression that you have a good solution already for making the ISO available to user. Chating with you on IRC, I got the impression that you dont. :( I don't think this feature is mergeable before we can make this feature actually work for users. So until I'm assured that you'll have a working and acceptable solution for this issue very soon (1-2 weeks), I'd be against merging this instead of bug#676537.
(In reply to comment #169) > I don't think this feature is mergeable before we can make this feature > actually work for users. So until I'm assured that you'll have a working and > acceptable solution for this issue very soon (1-2 weeks), I'd be against > merging this instead of bug#676537. Same goes for bug#676537 itself btw, I won't merge that in 3.6 branch until it has everything needed very soon.
(In reply to comment #169) > I was under the impression that you have a good solution already for making the > ISO available to user. Chating with you on IRC, I got the impression that you > dont. :( > > I don't think this feature is mergeable before we can make this feature > actually work for users. So until I'm assured that you'll have a working and > acceptable solution for this issue very soon (1-2 weeks), I'd be against > merging this instead of bug#676537. To summarize, to use the feature we need 1) an ISO with drivers (I have a python script mostly ready to generate it automatically at http://cgit.freedesktop.org/~teuf/spice-nsis/tree/tools ) 2) code in boxes to use the ISO (this is what this bug is about) 3) then we'll need some code to get the generated ISO on users' machines, and this is the part I haven't really thought about yet as I wanted to address the other 2 points first 2) is done unless I'm missing some unaddressed issues in these patches, I can finish 1) this week, but I don't think I'll make much progress on 3) this week. However, even without 3), the patches in this bug won't break since boxes will keep behaving as usual, and users wanted to exercise the new code can manually download an iso (just a crappy workaround, not something I'd be proud of ;) So I'd really like to get these patches in even if a bit more work is needed, better to work in incremental steps, and I'd hate to have this code bitrot once again.
(In reply to comment #171) > (In reply to comment #169) > > I was under the impression that you have a good solution already for making the > > ISO available to user. Chating with you on IRC, I got the impression that you > > dont. :( > > > > I don't think this feature is mergeable before we can make this feature > > actually work for users. So until I'm assured that you'll have a working and > > acceptable solution for this issue very soon (1-2 weeks), I'd be against > > merging this instead of bug#676537. > > To summarize, to use the feature we need > 1) an ISO with drivers (I have a python script mostly ready to generate it > automatically at http://cgit.freedesktop.org/~teuf/spice-nsis/tree/tools ) > 2) code in boxes to use the ISO (this is what this bug is about) > 3) then we'll need some code to get the generated ISO on users' machines, and > this is the part I haven't really thought about yet as I wanted to address the > other 2 points first Thanks for the explanation, appreciated! > 2) is done unless I'm missing some unaddressed issues in these patches, I can > finish 1) this week, but I don't think I'll make much progress on 3) this week. > > However, even without 3), the patches in this bug won't break since boxes will > keep behaving as usual, and users wanted to exercise the new code can manually > download an iso (just a crappy workaround, not something I'd be proud of ;) > So I'd really like to get these patches in even if a bit more work is needed, > better to work in incremental steps, and I'd hate to have this code bitrot once > again. Normally, I would agree completely with your approach here. However, I have to decide between these patches and bug#676537. We'll have to redo most of this work in libosinfo anyways and thats where 3) will have to be solved as I expect bug#676537 to be merged at least before you get to solve 3 (judging from the fact that you are not yet sure what the solution would look like).
(In reply to comment #172) > Normally, I would agree completely with your approach here. However, I have to > decide between these patches and bug#676537. Given that this patch series already had to be rebased several times because some other conflicting patch series were knowingly pushed first, I'd really appreciate if this time it got the nice no-conflict treatment... Moreover, bug#676537 really is 3.7 material at this point. https://live.gnome.org/ReleasePlanning/Freezes says "No new features can be added anymore so developers focus on stability" and "API freeze is not required for non-platform libraries, but is recommended.". While the changes in bug#676537 are not explicitly forbidden by any of this wording, they definitely go against the spirit of these freezes...
(In reply to comment #173) > (In reply to comment #172) > > > Normally, I would agree completely with your approach here. However, I have to > > decide between these patches and bug#676537. > > Given that this patch series already had to be rebased several times because > some other conflicting patch series were knowingly pushed first, I'd really > appreciate if this time it got the nice no-conflict treatment... If conflict was the only issue, I would give you an ACK but unfortunately thats not the case here. Its a half-baked solution that only and only works for users who are willing to go to a commandline to execute some script. Boxes is not even targetted for such users. > Moreover, bug#676537 really is 3.7 material at this point. > https://live.gnome.org/ReleasePlanning/Freezes says "No new features can be > added anymore so developers focus on stability" and "API freeze is not required > for non-platform libraries, but is recommended.". While the changes in > bug#676537 are not explicitly forbidden by any of this wording, they definitely > go against the spirit of these freezes... Don't care about the spirit, as long as I'm following the rules.
(In reply to comment #174) > If conflict was the only issue, I would give you an ACK but unfortunately thats > not the case here. Its a half-baked solution that only and only works for users > who are willing to go to a commandline to execute some script. Boxes is not > even targetted for such users. There are no scripts to execute, just a file to download in the right place, and I'm not saying this is an ideal solution, this is just a big step in the right direction. Once #1 and #2 are addressed, only #3 remains to be done, which can be done iteratively as #1 or #2 don't break anything if #3 is not done. Maintaining this code out of tree is quite costly, so better to merge this now that it's in a very good shape. Weren't you advocating getting code early and then refining it, rather than running after the perfect solution?
(In reply to comment #174) > Don't care about the spirit, as long as I'm following the rules. I guess I could dig up your various ACK, push your patch series now from IRC logs from 10 days ago, I probably wouldn't be violating any rule...
Turns out that there is some rather bigger open questions in regards to bug#676537 than I thought (the user avatar support) and it seems very unlikely they'll be solved this week so please go ahead and push these patches.
Sent a request to break feature freeze at https://mail.gnome.org/archives/gnome-boxes-list/2012-August/msg00013.html
Review of attachment 221395 [details] [review]: ::: src/iso-extractor.vala @@ +30,3 @@ + string? mount_point = GLib.DirUtils.mkdtemp (temp_name); + if (mount_point == null) + throw (GLib.IOError) new GLib.Error (G_IO_ERROR, g_io_error_from_errno (errno), "Failed to mount %s", temp_name); Slightly misleading error message - shouldn't this be "Failed to create mountpoint %s" ? @@ +35,3 @@ + debug ("Mounting '%s' on '%s'..", device_file, mount_point); + string[] argv = { "fuseiso", "-p", device_file, mount_point }; + yield exec (argv, cancellable); What if fuseiso is not available ? Do we get reasonable error handling in that case ?
Review of attachment 221396 [details] [review]: ::: src/iso-extractor.vala @@ +35,2 @@ debug ("Mounting '%s' on '%s'..", device_file, mount_point); + // the -p option tells fuseiso to rmdir the mountpoint on unmount This hunk seems unrelated to this commit, and should probably be moved to the previous one.
Review of attachment 221396 [details] [review]: ::: src/iso-extractor.vala @@ +35,2 @@ debug ("Mounting '%s' on '%s'..", device_file, mount_point); + // the -p option tells fuseiso to rmdir the mountpoint on unmount Ooops, bad rebase, thanks for catching this, fixed.
Review of attachment 221395 [details] [review]: ::: src/iso-extractor.vala @@ +30,3 @@ + string? mount_point = GLib.DirUtils.mkdtemp (temp_name); + if (mount_point == null) + throw (GLib.IOError) new GLib.Error (G_IO_ERROR, g_io_error_from_errno (errno), "Failed to mount %s", temp_name); Fixed @@ +35,3 @@ + debug ("Mounting '%s' on '%s'..", device_file, mount_point); + string[] argv = { "fuseiso", "-p", device_file, mount_point }; + yield exec (argv, cancellable); exec (from util.vala) will throw an exception when something bad happens. Then I have made sure that winxp-installer.vala falls back gracefully to not trying to use the additional drivers when an exception occurs. The other user is fedora-installer.vala, I haven't changed its error handling, hopefully it's ok too. I'll run some quick sanity check now to be sure.
I've now pushed all of this, thanks everyone!
Created attachment 223288 [details] [review] build: add Makefile rule to download extra Win drivers ISO Since a4a0780c8, Boxes is able to use an ISO with extra Windows drivers to auto-install the viostor virtio drivers during Windows unattended installations. However, it does not know yet how to download that ISO. As a stop-gap measure, add a Makefile rule to download it and save it with the right name. It can then be copied to ~/.cache/gnome-boxes/ or to $data_dir/gnome-boxes/, and Boxes will be able to make use of it.
Review of attachment 223288 [details] [review]: ack
Attachment 223288 [details] pushed as af09bc7 - build: add Makefile rule to download extra Win drivers ISO