Hi Mark, On Sat, 2007-06-09 at 14:51 +0100, Mark McLoughlin wrote: > - Including vcpu, memory, graphics and nic in this metadata is mixing > up two things - the things the image need in order to boot and the > defaults recommended when instantiating a guest with the image. Agreed; that would be much cleaner. I'll revise the patches > - The disk -> target mapping thing is pretty strange - I'd suggest > that the order the disks are listed in just specifies the target. In the code, specifying a target is actually optional; it just seemed more understandable to me to allow explicit mappings. > Is it hd/cdrom, make sure the boot disk is the first disk etc. > > <loader disk="boot-hvm.img" /> Doesn't the CDROM always need to be hdc ? (at least that's what the current virt-install code does) With that, you'd have some funky out-of-order mapping. > - You don't have a human readable name for a UI that allows people to > choose from a number of images. Oh, yeah; I'll add a label and description. > - From what I can see, you still have the ability to create a disk at > instantiation time, but not format it? The more I think about it, the less this whole notion of 'instantiate disk automatically' seems to make sense to me; will appliances really detect that they'd been given a blank disk and then partition and format it ? It'd probably make more sense if the metadata could express what partitions with what FS's should be on disks - then auto-creation of scratch or user disks could actually be useful for appliance builders. > - If you added ImageInstaller support to virt-install, couldn't > virt-image almost just run virt-install rather than re-implementing > a lot of it? i.e. at a glance, it looks like you've forked > virt-install in order to have a version with a simpler command > line. I don't have a problem with virt-image, and the simpler > command line, it's the copied and pasted code I don't like. Yeah, I had that same uneasy feeling; I'll go through and factor out as much as I can. > - This looks odd: > > + order = [ "xen", "kvm", "kqemu", "qemu" ] > + for o in order: > + if types.count(o) > 0: > > i.e. it'd be better not to have to keep that list updated. How > about you just pick the first type? If libvirt doesn't order the > list of available types in a useful order, maybe it should? Agreed; going just by the order in the metadata seems cleaner. If we do that, then there's no need for libvirt ordering the types. > - You don't have a way of specifying the disk image should be > read-only. Agreed, I'll try and stick that in there. > - Shouldn't we be copying even system disk images, unless they're > read-only, on instantiation - i.e. you should be able to run > virt-image multiple times. I kinda left the issue of multiple instantiations of images out on purpose, to keep the first cut simple, since they require a good deal of book-keeping, too, to make image-based updates possible. In any event, I think the user should still be able to override if copies are made or not; to make this really usable you'd probably also want ot support an image format that can do snapshot-based copies like qcow. At least on my LAN, copying a few hundred megs of disk images around is no fun :( David
