Created attachment 205223 [details] [review]
express: No need for floppy image

libvirt+qemu can simply expose a directory as a device so we don't
really need to keep/create a floppy image for each express installation.

Created attachment 205225 [details] [review]
express: No need for floppy image

libvirt+qemu can simply expose a directory as a device so we don't
really need to keep/create a floppy image for each express installation.

Created attachment 205226 [details] [review]
express: No need for floppy image

libvirt+qemu can simply expose a directory as a device so we don't
really need to keep/create a floppy image for each express installation.

Created attachment 205318 [details] [review]
express: No need for floppy image

libvirt+qemu can simply expose a directory as a device so we don't
really need to keep/create a floppy image for each express installation.

Review of attachment 205318 [details] [review]:

::: src/fedora-installer.vala
@@ +102,3 @@
     }
+
+    private async void exec (string[] argv, Cancellable? cancellable) throws GLib.Error {

can you move these two exec() functions to util.vala?

(In reply to comment #5)
> Review of attachment 205318 [details] [review]:
> 
> ::: src/fedora-installer.vala
> @@ +102,3 @@
>      }
> +
> +    private async void exec (string[] argv, Cancellable? cancellable) throws
> GLib.Error {
> 
> can you move these two exec() functions to util.vala?

Yup!

Created attachment 205362 [details] [review]
express: No need for floppy image

libvirt+qemu can simply expose a directory as a device so we don't
really need to keep/create a floppy image for each express installation.

Review of attachment 205362 [details] [review]:

ack

After this small chat on IRC, I'm no longer excited about this :(


<danpb> i hope you're not using the QEMU feature to emulate a FAT formated image, from a directory ?
<danpb> zeenix: oh dear god - you *really* don't want to be using that feature
* elmarco_ (~mlureau@nat-pool-rdu.redhat.com) has joined #boxes
<danpb> the QEMU developers hate it and RHEL KVM maintainers refuse to build that code in RHEL
* zeenix commits suicide
<danpb> you're much much better generating a temporary file to serve as a real FAT image
<danpb> we only added that to libvirt because VDSM used to need it, but they've stopped using it now
<zeenix> but seems like a pretty nice feature
<danpb> really badly implemented in QEMU and very crash prone

Attachment 205362 [details] pushed as bc80627 - express: No need for floppy image

(In reply to comment #10)
> Attachment 205362 [details] pushed as bc80627 - express: No need for floppy image

Just to be clear, I didn't exactly push this patch. It was a git-bz failure on my part. The patch I pushed was modified to contain justification/explanation of the concert above:

    According to Paolo Bonzini, we are safe to depend on this qemu feature
    as long as we use the disk read-only and thats what we do.

This breaks autoinstall for me:
qemu-kvm: -drive file=fat:floppy:/home/teuf/.cache/gnome-boxes/win7-unattended,if=none,id=drive-fdc0-0-0,readonly=on: could not open disk image fat:floppy:/home/teuf/.cache/gnome-boxes/win7-unattended: Operation not permitted

Autoinstall works again after reverting it.

Please do not push such a fundamental change in autoinstalls literally hours before releasing :-/

(In reply to comment #12)
> This breaks autoinstall for me:
> qemu-kvm: -drive
> file=fat:floppy:/home/teuf/.cache/gnome-boxes/win7-unattended,if=none,id=drive-fdc0-0-0,readonly=on:
> could not open disk image
> fat:floppy:/home/teuf/.cache/gnome-boxes/win7-unattended: Operation not
> permitted
> 
> Autoinstall works again after reverting it.
> Please do not push such a fundamental change in autoinstalls literally hours
> before releasing :-/

I tested it (on my machine of course cause i don't have access to yours :)), it was ACKED and has been floating around in bugzilla for a while now so its not like i just came-up with it and pushed it without giving it any thoughts. Anyways, I'll revert it. Thanks for testing!

commit: 12b120f 12b120fded5ca91468c6ac0ab6e03ca247da2cee
Author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>
Date:   Mon Feb 6 20:31:21 2012 +0100

    Revert "express: No need for floppy image"
    
    This reverts commit bc806277b17856543bfeb5f07c7de66c3db5012d.
    
    Turns out that this really breaks against qemu builds in fedora at
    least. So reverting this for now.

(In reply to comment #13)> 
> it was ACKED

the general idea and the boxes code got ACKed, and then you got a very strong NACK for the libvirt/qemu side... Which is in my opinion a strong hint that this is something that needs special attention and careful testing (by "testing" I mean checking that it works with multiple guest and host OS combinations)

> it has been floating around in bugzilla for a while now

Well, with respect to getting testers, being in bugzilla for a while or appearing suddenly out of the blue in git is unfortunately the same most of the time.

I was asked on IRC about specifics how to reproduce this bug, totally forgot to answer: this was tested on a x86_64 host with a 32 bit windows 7 iso

(In reply to comment #16)
> I was asked on IRC about specifics how to reproduce this bug, totally forgot to
> answer: this was tested on a x86_64 host with a 32 bit windows 7 iso

I'm interested to know: which distro? Which qemu version ? self-built or from from distro?

Right sorry, I knew I was forgetting some stuff. Latest qemu-kvm package from f16 repos, gnome-boxes built through jhbuild.

(In reply to comment #18)
> Right sorry, I knew I was forgetting some stuff. Latest qemu-kvm package from
> f16 repos, gnome-boxes built through jhbuild.

Interesting! I wonder how autoinstall of any windows works for you then cause it has been broken for quite some time now due to floppy support broken in there.

It's fixed in updates-testing http://koji.fedoraproject.org/koji/buildinfo?buildID=296381

Still relevant? I am not sure we want to rely on less tested qemu feature, since we already have a solution using the more robust approach...

(In reply to comment #21)
> Still relevant? I am not sure we want to rely on less tested qemu feature,
> since we already have a solution using the more robust approach...

Nah, not worth it i guess..