[et-mgmt-tools] [PATCH] virt-install: use --disk to specify size and sparse

[Cole Robinson <crobinso redhat com>]

The attached patch expands the options that can be
lumped onto virt-install's new --disk option to
specify size of new storage, and whether we want to
create it as sparse.

The format for these options is:

  --disk path=/some/new/file,size=5,sparse=true|false

This acts similar to the current options: if the file
doesn't exist, size is required, and we default to
sparse=True if it isn't specified.

This effectively deprecates --file, --file-size, and
--nonsparse, and in my opinion is much simpler. It also
helps overcome some problems we had with the original
options for cases of specifying multiple disks: there
was no way to specify one sparse disk and one nonsparse
disk, and generally specifying multiple disks was
unclear and a pain.

I also added a manpage section for the --disk option.

Any comments appreciated.

Thanks,
Cole

diff -r c20df049889e man/en/virt-install.pod
--- a/man/en/virt-install.pod	Fri Sep 05 09:15:55 2008 -0400
+++ b/man/en/virt-install.pod	Fri Sep 05 10:15:39 2008 -0400
@@ -1,4 +1,4 @@
-=od
+=pod
 
 =head1 NAME
 
@@ -80,13 +80,82 @@
 If the value 'auto' is passed, virt-install attempts to automatically determine
 an optimal cpu pinning using NUMA data, if available.
 
+=item --disk=DISKOPTS
+
+Specifies media to use as storage for the guest, with various options. The
+general format of a disk string is
+
+    --disk opt1=val1,opt2=val2,...
+
+To specify media, one of the following options is required:
+
+=over 4
+
+=item B<path>
+
+A path to some storage media to use, existing or not. Existing media can be
+a file or block device. If installing on a remote host, the existing media
+must be shared as a libvirt storage volume.
+
+Specifying a non-existent path implies attempting to create the new storage,
+and will require specifyng a 'size' value. If the base directory of the path
+is a libvirt storage pool on the host, the new storage will be created as a
+libvirt storage volume. For remote hosts, the base directory is required to be
+a storage pool if using this method.
+
+=item B<pool>
+
+An existing libvirt storage pool name to create new storage on. Requires
+specifying a 'size' value.
+
+=item B<vol>
+
+An existing libvirt storage volume to use. This is specified as
+'poolname:volname'.
+
+=back
+
+Other available options:
+
+=over 4
+
+=item B<device>
+
+Disk device type. Value can be 'cdrom', 'disk', or 'floppy'. Default is
+'disk'. If a 'cdrom' is specified, and no install method is chosen, the
+cdrom is used as the install media.
+
+=item B<perms>
+
+Disk permissions. Value can be 'rw' (Read/Write), 'ro' (Readonly),
+or 'sh' (Shared Read/Write). Default is 'rw'
+
+=item B<size>
+
+size (in GB) to use if creating new storage
+
+=item B<sparse>
+
+whether to skip fully allocating newly created storage. Value is 'true' or
+'false'. Default is 'true' (do not fully allocate).
+
+=back
+
+See the examples section for some uses. This option deprecates C<--file>,
+C<--file-size>, and C<--nonsparse>.
+
+=item --nodisks
+
+Request a virtual machine without any local disk storage, typically used for
+running 'Live CD' images or installing to network storage (iSCSI or NFS root).
+
 =item -f DISKFILE, --file=DISKFILE
 
 Path to the file, disk partition, or logical volume to use as the backing store
 for the guest's virtual disk. If the path does not exist, then C<--file-size>
 option should also be specified, allowing the disk to be created. This parameter
-can be repeated multiple times to add many disk. This parameter will be prompted 
-for if omitted on the command line. 
+can be repeated multiple times to add many disk. This option is deprecated in
+favor of --disk.
 
 =item -s DISKSIZE, --file-size=DISKSIZE
 
@@ -103,12 +172,6 @@
 by faster install times inside the guest. Thus use of this optional is recommended 
 to ensure consistently high performance and to avoid I/O errors in the guest 
 should the host filesystem fill up. 
-
-=item --nodisks
-
-Request a virtual machine without any local disk storage, typically used for 
-running 'Live CD' images or installing to network storage (iSCSI or NFS root).
-This disables all interactive prompts for disk setup.
 
 =item -m MAC, --mac=MAC
 
@@ -248,9 +311,9 @@
 File to use a virtual CD-ROM device for fully virtualized guests. It can be
 path to an ISO image, or to a CDROM device. It can also be a URL from which
 to fetch/access a minimal boot ISO image. The URLs take the same format as
-described for the C<--location> argument. If this parameter is omitted then
-the C<--location> argument must be given to specify a location for the kernel
-and initrd, or the C<--pxe> argument used to install from the network.
+described for the C<--location> argument. If a cdrom has been specified via
+the C<--disk> option, and neither C<--cdrom> nor any other install option is
+specified, the C<--disk> cdrom is used as the install media.
 
 =item  --pxe
 
@@ -504,8 +567,60 @@
 
 =head1 EXAMPLES
 
+Install a KVM guest, creating a new storage file, virtual networking,
+booting from the host CDROM, using VNC server/viewer
+
+  # virt-install \
+       --connect qemu:///system \
+       --name demo \
+       --ram 500 \
+       --disk path=/var/lib/libvirt/images/demo.img,size=5 \
+       --network network:default \
+       --accelerate \
+       --vnc \
+       --cdrom /dev/cdrom
+
+
+Install a Fedora 9 KVM guest, using LVM partition, virtual networking,
+booting from PXE, using VNC server/viewer
+
+  # virt-install \
+       --connect qemu:///system \
+       --name demo \
+       --ram 500 \
+       --disk path=/dev/HostVG/DemoVM \
+       --network network:default \
+       --accelerate \
+       --vnc \
+       --os-variant fedora9
+
+Install a QEMU guest, with a real partition, for a different architecture
+using SDL graphics, using a remote kernel and initrd pair:
+
+  # virt-install \
+       --connect qemu:///system \
+       --name demo \
+       --ram 500 \
+       --disk path=/dev/hdc \
+       --network bridge:eth1 \
+       --arch ppc64 \
+       --sdl \
+       --location http://download.fedora.redhat.com/pub/fedora/linux/core/6/x86_64/os/
+
+Run a Live CD image under Xen fullyvirt, in diskless environment
+
+  # virt-install \
+       --hvm \
+       --name demo \
+       --ram 500 \
+       --nodisks \
+       --livecd \
+       --vnc \
+       --cdrom /root/fedora7live.iso
+
 Install a paravirtualized Xen guest, 500 MB of RAM, a 5 GB of disk, and
-Fedora Core 6 from a web server, in text-only mode:
+Fedora Core 6 from a web server, in text-only mode, with old style --file
+options:
 
   # virt-install \
        --paravirt \
@@ -516,67 +631,23 @@
        --nographics \
        --location http://download.fedora.redhat.com/pub/fedora/linux/core/6/x86_64/os/
 
-Install a QEMU guest, with a real partition, for a different architecture
-using SDL graphics, using a local ISO image:
-
-  # virt-install \
-       --connect qemu:///system \
-       --name demo \
-       --ram 500 \
-       --file /dev/hdc \
-       --network bridge:eth1 \
-       --arch ppc64 \
-       --sdl \
-       --cdrom /root/boot.iso
-
-Install a QEMU guest, with a real partition, for a different architecture
-using SDL graphics, using a remote kernel and initrd pair:
-
-  # virt-install \
-       --connect qemu:///system \
-       --name demo \
-       --ram 500 \
-       --file /dev/hdc \
-       --network bridge:eth1 \
-       --arch ppc64 \
-       --sdl \
-       --location http://download.fedora.redhat.com/pub/fedora/linux/core/6/x86_64/os/
-
-Install a KVM guest, using LVM partition, virtual networking, booting from
-the host CDROM, using VNC server/viewer
-
-  # virt-install \
-       --connect qemu:///system \
-       --name demo
-       --ram 500 \
-       --file /dev/HostVG/DemoVM \
-       --network network:default \
-       --accelerate \
-       --vnc \
-       --cdrom /dev/cdrom 
-
-Run a Live CD image under Xen fullyvirt, in diskless environment
-
-  # virt-install \
-       --hvm \
-       --name demo \
-       --ram 500 \
-       --nodisk \
-       --livecd \
-       --vnc \
-       --cdrom /root/fedora7live.iso
 
 =head1 AUTHOR
 
-Written by Daniel P. Berrange, Hugh Brock, Jeremy Katz and a team of many
-other contributors. See the AUTHORS file in the source distribution for
-the complete list of credits.
+Written by Daniel P. Berrange, Hugh Brock, Jeremy Katz, Cole Robinson and a
+team of many other contributors. See the AUTHORS file in the source
+distribution for the complete list of credits.
 
 =head1 BUGS
 
-Report bugs to the mailing list C<http://www.redhat.com/mailman/listinfo/et-mgmt-tools>
+Report bugs to the mailing list
+C<http://www.redhat.com/mailman/listinfo/et-mgmt-tools>
 or directly to BugZilla C<http://bugzilla.redhat.com/bugzilla/> against the
 C<Fedora> product, and the C<python-virtinst> component.
+
+When filing a bug, please run the failing command with the --debug command
+line flag and post the output to the bug report, along with
+$HOME/.virtinst/virt-install.log
 
 =head1 COPYRIGHT
 
diff -r c20df049889e virt-install
--- a/virt-install	Fri Sep 05 09:15:55 2008 -0400
+++ b/virt-install	Fri Sep 05 10:15:39 2008 -0400
@@ -52,6 +52,7 @@
     devtype = None
     ro = False
     shared = False
+    sparse = True
 
     origpath = path
 
@@ -66,27 +67,44 @@
         fail(_("--disk path must start with path=, pool=, or vol=."))
     path = path[len(path_type):]
 
-    # Parse endings for perms and device type
+    # Parse out comma separated options
+    opts = {}
     while True:
         if not path.count(","):
             break
-        path, opts = path.split(",", 1)
-        for opt in opts.split(","):
+        path, tmpopts = path.split(",", 1)
+        for opt in tmpopts.split(","):
             opt_type = None
             opt_val = None
             if opt.count("="):
                 opt_type, opt_val = opt.split("=", 1)
-            if opt_type == "device":
-                devtype = opt_val
-            elif opt_type == "perms":
-                if opt_val == "ro":
-                    ro = True
-                elif opt_val == "sh":
-                    shared = True
-                else:
-                    fail(_("Unknown perms value '%s'." % opt_val))
+                opts[opt_type.lower()] = opt_val.lower()
+
+    for opt in opts.items():
+        opt_type, opt_val = opt
+        if opt_type == "device":
+            devtype = opt_val
+        elif opt_type == "perms":
+            if opt_val == "ro":
+                ro = True
+            elif opt_val == "sh":
+                shared = True
             else:
-                fail(_("Unknown disk option '%s'." % opt))
+                fail(_("Unknown '%s' value '%s'" % (opt_type, opt_val))
+        elif opt_type == "size":
+            try:
+                size = float(opt_val)
+            except Exception, e:
+                fail(_("Improper value for 'size': %s" % str(e)))
+        elif opt_type == "sparse":
+            if opt_val == "true":
+                sparse = True
+            elif opt_val == "false":
+                sparse = False
+            else:
+                fail(_("Unknown '%s' value '%s'") % (opt_type, opt_val))
+        else:
+            fail(_("Unknown --disk option '%s'." % opt))
 
     # We return (path, (poolname, volname), volinst, device, readonly, shared)
     if path_type == "path=":
@@ -112,7 +130,7 @@
 
     if not devtype:
         devtype = virtinst.VirtualDisk.DEVICE_DISK
-    ret = (abspath, voltuple, volinst, devtype, ro, shared)
+    ret = (abspath, voltuple, volinst, devtype, ro, shared, size, sparse)
     logging.debug("parse_disk: returning %s" % str(ret))
     return ret
 
@@ -121,13 +139,14 @@
     try:
         # Get disk parameters
         if is_file_path:
-            (path, voltuple, volinst,
-             device, readOnly, shared) = (disk, None, None,
-                                         virtinst.VirtualDisk.DEVICE_DISK,
-                                         False, False)
+            (path, voltuple, volinst, device, readOnly, shared, size,
+             sparse) = \
+             (disk, None, None, virtinst.VirtualDisk.DEVICE_DISK, False,
+              False, size, sparse)
         else:
             (path, voltuple, volinst,
-             device, readOnly, shared) = parse_disk_option(guest, disk, size)
+             device, readOnly, shared,
+             size, sparse) = parse_disk_option(guest, disk, size)
             if not sparse and volinst:
                 volinst.allocation = volinst.capacity
 
@@ -162,8 +181,9 @@
         if file_paths or disk_paths or size:
             fail(_("Cannot use --file, --size, or --disk with --nodisks"))
         return
-    if file_paths and disk_paths:
-        fail(_("Cannot mix --file and --disk options."))
+    if (file_paths or size or sparse == False) and disk_paths:
+        fail(_("Cannot mix --file, --nonsparse, or --file-size with --disk "
+               "options. Please see the manual for --disk syntax."))
     elif not file_paths and not disk_paths:
         fail(_("A disk must be specified (use --nodisks to override)"))
 
@@ -333,13 +353,13 @@
 
     parser.add_option_group(insg)
     stog = OptionGroup(parser, _("Storage Configuration"))
+    stog.add_option("", "--disk", type="string", dest="diskopts",
+                    action="callback", callback=cli.check_before_append,
+                    help=_("Specify storage to use as a disk with various "
+                           "options."))
     stog.add_option("-f", "--file", type="string", dest="file_path",
                     action="callback", callback=cli.check_before_append,
                     help=_("File to use as the disk image"))
-    stog.add_option("", "--disk", type="string", dest="disk_path",
-                    action="callback", callback=cli.check_before_append,
-                    help=_("Specify storage to use as a disk with various "
-                           "options."))
     stog.add_option("-s", "--file-size", type="float",
                     action="append", dest="disksize",
                     help=_("Size of the disk image (if it doesn't exist) in "
@@ -505,7 +525,7 @@
         cli.get_sound(options.sound, guest)
 
     # set up disks
-    get_disks(options.file_path, options.disk_path, options.disksize,
+    get_disks(options.file_path, options.diskopts, options.disksize,
               options.sparse, options.nodisks, guest, hvm, conn)
 
     # set up network information