Joey Boggs wrote:
> Updated virt-pack and Unware.py per suggestions
>
Comments inline:
> diff -Naur virtinst--devel.orig/virtinst/UnWare.py virtinst--devel/virtinst/UnWare.py
> --- virtinst--devel.orig/virtinst/UnWare.py 1969-12-31 19:00:00.000000000 -0500
> +++ virtinst--devel/virtinst/UnWare.py 2008-06-09 13:03:41.000000000 -0400
> @@ -0,0 +1,309 @@
> +#
> +# Processing of VMWare(tm) .vmx files
> +#
> +# Copyright 2007 Red Hat, Inc.
> +# David Lutterkort <dlutter@xxxxxxxxxx>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> +# MA 02110-1301 USA.
> +
> +import time
> +import sys
> +import os
> +
> +import ImageParser
> +import util
> +
> +class Disk:
> + """A disk for a VMWare(tm) virtual machine"""
> +
> + MONOLITHIC_FLAT = "monolithicFlat"
> + TWO_GB_MAX_EXTENT_SPARSE = "twoGbMaxExtentSparse"
> + # This seems only to be usable if the vmdk header is embedded in the
> + # data file, not when the descriptor is in a separate text file. Use
> + # TWO_GB_MAX_EXTENT_SPARSE instead.
> + # VMWare's(tm) documentation of VMDK seriously sucks. A lot.
> + MONOLITHIC_SPARSE = "monolithicSparse"
> +
> + IDE_HEADS = 16
> + IDE_SECTORS = 63
> +
> + def __init__(self, descriptor, extent, size, dev, format):
> + """Create a new disk. DESCRIPTOR is the name of the VMDK descriptor
> + file. EXTENT is the name of the file holding the actual data. SIZE
> + is the filesize in bytes. DEV identifies the device, for IDE (the
> + only one supported right now) it should be $bus:$master. FORMAT is
> + the format of the underlying extent, one of the formats defined in
> + virtinst.ImageParser.Disk"""
> + self.cid = 0xffffffff
> + self.createType = Disk.MONOLITHIC_FLAT
> + self.descriptor = descriptor
> + self.extent = extent
> + self.size = size
> + self.dev = dev
> + self.format = format
> +
> + def make_extent(self, base):
> + """Write the descriptor file, and create the extent as a monolithic
> + sparse extent if it does not exist yet"""
> + f = os.path.join(base, self.extent)
> + print "Checking %s" % f
This print is still in place, any reason for that? My previous comment was:
> We shouldn't just straight 'print' from a library, even if only one app is using it.
> I suggest using logging.debug.
> + if not os.path.exists(f):
> + util.system("qemu-img create -f vmdk %s %d" % (f, self.size/1024))
> + self.createType = Disk.TWO_GB_MAX_EXTENT_SPARSE
> + else:
> + qemu = os.popen("qemu-img info %s" % f, "r")
> + for l in qemu:
> + (tag, val) = l.split(":")
> + if tag == "file format" and val.strip() == "vmdk":
> + self.createType = Disk.TWO_GB_MAX_EXTENT_SPARSE
> + qemu.close()
> + return self.extent
> +
> + def _VMDK_TEMPLATE(self):
> +
> + blocks = self.size/512
> + if self.createType == Disk.MONOLITHIC_FLAT:
> + vmdk_extent_info= "RW %d FLAT \"%s\" 0\n" % (blocks, os.path.basename(self.extent))
> + else: # Disk.MONOLITHIC_SPARSE
> + vmdk_extent_info= "RW %d SPARSE \"%s\"\n" % (blocks, os.path.basename(self.extent))
> +
> + dict = {
> + "SELF_CID" : self.cid,
> + "CREATE_TYPE" : self.createType,
> + "IDE_SECTORS" : Disk.IDE_SECTORS,
> + "IDE_HEADS" : Disk.IDE_HEADS,
> + "IDE_BLOCKS" : blocks,
> + "IDE_CYLINDERS" : blocks/(Disk.IDE_SECTORS*Disk.IDE_HEADS),
> + "VMDK_EXTENT_INFO" : vmdk_extent_info,
> + }
> +
> + vmdk = """# Disk DescriptorFile
> +# Generated from virtinst
> +version=1
> +
> +CID=%(SELF_CID)s
> +parentCID=ffffffff
> +createType="%(CREATE_TYPE)s"
> +
> +# Extent description
> +%(VMDK_EXTENT_INFO)s
> +
> +# Disk Data Base
> +ddb.virtualHWVersion = "4"
> +ddb.adapterType = "ide"
> +ddb.geometry.sectors = "%(IDE_SECTORS)s"
> +ddb.geometry.heads = "%(IDE_HEADS)s"
> +ddb.geometry.cylinders = "%(IDE_CYLINDERS)s"
> +"""
> + vmdk = vmdk % dict
> + return vmdk
Not that there are any adverse effects with the current code, but
'dict' is actually used in python referring to the actual
dictionary type. So it might be good to use a different variable
name here (and there is an example below as well.)
> +
> +
> + def to_vmx(self):
> + """Return the fragment for the VMX file for this disk"""
> +
> + vmx = ""
> + dict = {
> + "dev" : self.dev,
> + "disk_filename" : self.descriptor
> + }
> + if self.format == ImageParser.Disk.FORMAT_ISO:
> + vmx = _VMX_ISO_TEMPLATE % dict
> + else: # FORMAT_RAW
> + vmx = _VMX_IDE_TEMPLATE % dict
> + return vmx
> +
> +class Image:
> + """Represent an image for generation of a VMWare(tm) description"""
> +
> + def __init__(self, image = None):
> + if image is not None:
> + self._init_from_image(image)
> +
> + def _init_from_image(self, image):
> + domain = image.domain
> + boot = domain.boots[0]
> +
> + self.base = image.base
> + self.name = image.name
> + self.descr = image.descr
> + self.label = image.label
> + self.vcpu = domain.vcpu
> + self.memory = domain.memory
> + self.interface = domain.interface
> +
> + self.disks = []
> + for d in boot.drives:
> + disk = d.disk
> + descriptor = sub_ext(disk.file, ".vmdk")
> + if disk.size is None:
> + f = os.path.join(image.base, disk.file)
> + size = os.stat(f).st_size
> + else:
> + size = long(disk.size) * 1024L * 1024L
> + ide_count = len(self.disks)
> + dev = "%d:%d" % (ide_count / 2, ide_count % 2)
> + self.disks.append(Disk(descriptor, disk.file, size, dev,
> + disk.format))
> +
> + def make(self, base):
> + """Write the descriptor file and all the disk descriptors"""
> + files = []
> + out = open(os.path.join(self.base, self.name + ".vmx"), "w")
> + out.write(self.to_vmx())
> + out.close()
> + files.append(self.name + ".vmx")
> +
> + for d in self.disks:
> + f = d.make_extent(self.base)
> + files.append(f)
> + out = open(os.path.join(base, d.descriptor), "w")
> + out.write(d._VMDK_TEMPLATE())
> + out.close()
> + files.append(d.descriptor)
> + return files
> +
> + def to_vmx(self):
> + """Return the VMX description of this image"""
> + # Strip blank spaces and EOL to prevent syntax errors in vmx file
> + self.descr = self.descr.strip()
> + self.descr = self.descr.replace("\n","|")
> +
> + dict = {
> + "now": time.strftime("%Y-%m-%dT%H:%M:%S %Z", time.localtime()),
> + "progname": os.path.basename(sys.argv[0]),
> + "/image/name": self.name,
> + "/image/description": self.descr or "None",
> + "/image/label": self.label or self.name,
> + "/image/devices/vcpu" : self.vcpu,
> + "/image/devices/memory": long(self.memory)/1024
> + }
> +
> + vmx = _VMX_MAIN_TEMPLATE % dict
> + if self.interface:
> + vmx += _VMX_ETHER_TEMPLATE
> +
> + for d in self.disks:
> + vmx += d.to_vmx()
> +
> + return vmx
> +
> +def sub_ext(filename, ext):
> + return os.path.splitext(filename)[0] + ext
> +
> +_VMX_MAIN_TEMPLATE = """
> +#!/usr/bin/vmplayer
> +
> +# Generated %(now)s by %(progname)s
> +# http://virt-manager.et.redhat.com/
> +
> +# This is a Workstation 5 or 5.5 config file
> +# It can be used with Player
> +config.version = "8"
> +virtualHW.version = "4"
> +
> +# Selected operating system for your virtual machine
> +guestOS = "other"
> +
> +# displayName is your own name for the virtual machine
> +displayName = "%(/image/name)s"
> +
> +# These fields are free text description fields
> +annotation = "%(/image/description)s"
> +guestinfo.vmware.product.long = "%(/image/label)s"
> +guestinfo.vmware.product.url = "http://virt-manager.et.redhat.com/";
> +guestinfo.vmware.product.class = "virtual machine"
> +
> +# Number of virtual CPUs. Your virtual machine will not
> +# work if this number is higher than the number of your physical CPUs
> +numvcpus = "%(/image/devices/vcpu)s"
> +
> +# Memory size and other memory settings
> +memsize = "%(/image/devices/memory)d"
> +MemAllowAutoScaleDown = "FALSE"
> +MemTrimRate = "-1"
> +
> +# Unique ID for the virtual machine will be created
> +uuid.action = "create"
> +
> +## For appliances where tools are installed already, this should really
> +## be false, but we don't have that ionfo in the metadata
> +# Remind to install VMware Tools
> +# This setting has no effect in VMware Player
> +tools.remindInstall = "TRUE"
> +
> +# Startup hints interfers with automatic startup of a virtual machine
> +# This setting has no effect in VMware Player
> +hints.hideAll = "TRUE"
> +
> +# Enable time synchronization between computer
> +# and virtual machine
> +tools.syncTime = "TRUE"
> +
> +# First serial port, physical COM1 is not available
> +serial0.present = "FALSE"
> +
> +# Optional second serial port, physical COM2 is not available
> +serial1.present = "FALSE"
> +
> +# First parallell port, physical LPT1 is not available
> +parallel0.present = "FALSE"
> +
> +# Logging
> +# This config activates logging, and keeps last log
> +logging = "TRUE"
> +log.fileName = "%(/image/name)s.log"
> +log.append = "TRUE"
> +log.keepOld = "3"
> +
> +# These settings decides interaction between your
> +# computer and the virtual machine
> +isolation.tools.hgfs.disable = "FALSE"
> +isolation.tools.dnd.disable = "FALSE"
> +isolation.tools.copy.enable = "TRUE"
> +isolation.tools.paste.enabled = "TRUE"
> +
> +# Settings for physical floppy drive
> +floppy0.present = "FALSE"
> +"""
> +
> +_VMX_ETHER_TEMPLATE = """
> +## if /image/devices/interface is present:
> +# First network interface card
> +ethernet0.present = "TRUE"
> +ethernet0.connectionType = "nat"
> +ethernet0.addressType = "generated"
> +ethernet0.generatedAddressOffset = "0"
> +ethernet0.autoDetect = "TRUE"
> +"""
> +
> +_VMX_ISO_TEMPLATE = """
> +# CDROM drive
> +ide%(dev)s.present = "TRUE"
> +ide%(dev)s.deviceType = "cdrom-raw"
> +ide%(dev)s.startConnected = "TRUE"
> +ide%(dev)s.fileName = "%(disk_filename)s"
> +ide%(dev)s.autodetect = "TRUE"
> +"""
> +
> +_VMX_IDE_TEMPLATE = """
> +# IDE disk
> +ide%(dev)s.present = "TRUE"
> +ide%(dev)s.fileName = "%(disk_filename)s"
> +ide%(dev)s.mode = "persistent"
> +ide%(dev)s.startConnected = "TRUE"
> +ide%(dev)s.writeThrough = "TRUE"
> +"""
> diff -Naur virtinst--devel.orig/virt-pack virtinst--devel/virt-pack
> --- virtinst--devel.orig/virt-pack 1969-12-31 19:00:00.000000000 -0500
> +++ virtinst--devel/virt-pack 2008-06-09 13:03:30.000000000 -0400
> @@ -0,0 +1,146 @@
> +#!/usr/bin/python -tt
> +#
> +# Package and unpackage images for distribution
> +#
> +# Copyright 2007 Red Hat, Inc.
> +# David Lutterkort <dlutter@xxxxxxxxxx>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> +# MA 02110-1301 USA.
> +
> +import os, sys, string
> +from optparse import OptionParser, OptionValueError
> +import subprocess
> +import logging
> +import libxml2
> +import urlgrabber.progress as progress
> +
> +import virtinst
> +import virtinst.ImageParser
> +import virtinst.CapabilitiesParser
> +import virtinst.cli as cli
> +import virtinst.util as util
> +import virtinst.UnWare
> +
> +import tempfile
> +
> +import gettext
> +import locale
> +
> +locale.setlocale(locale.LC_ALL, '')
> +gettext.bindtextdomain(virtinst.gettext_app, virtinst.gettext_dir)
> +gettext.install(virtinst.gettext_app, virtinst.gettext_dir)
> +
> +class PackageException(Exception):
> + def __init__(self, msg):
> + Exception.__init__(self, msg)
> +
> +class Package:
> +
> + def __init__(self, image):
> + self.image = image
> + if image.name is None or image.version is None:
> + raise PackageException(
> + _("The image name and version must be present"))
> + self.vername = "%s-%s" % (image.name, image.version)
> + self.tmpdir = tempfile.mkdtemp(dir="/var/tmp", prefix="virt-pack")
> + self.stagedir = os.path.join(self.tmpdir, self.vername)
> + self.files = []
> +
> + def add_image_files(self):
> + cwd = os.getcwd()
> + img = self.image
> + self.files.append(os.path.basename(img.filename))
> + try:
> + os.chdir(img.base)
> + for d in img.storage.keys():
> + disk = img.storage[d]
> + if disk.use == disk.USE_SCRATCH:
> + if disk.size is None:
> + raise PackageException(_("Scratch disk %s does not have a size attribute") % disk.id)
> + else:
> + if not os.path.exists(disk.file):
> + raise PackageException(_("Disk file %s could not be found") % disk.id)
> + self.files.append(disk.file)
> + finally:
> + os.chdir(cwd)
> +
Is there any reason for this to still be here that I'm missing? My previous
comment was:
> Why change directories here? Seems like it could all be avoided if the
> above check is changed to: os.path.exists(os.path.join(img.base, disk.file))
> + def make_vmx_files(self):
> + img = virtinst.UnWare.Image(self.image)
> + files = img.make(self.image.base)
> + self.files.extend(files)
> +
> + def pack(self, outdir):
> + outfile = os.path.join(outdir, self.vername + ".tgz")
> + for f in set(self.files):
> + dir = os.path.join(self.stagedir, os.path.dirname(f))
> + if not os.path.exists(dir):
> + os.makedirs(dir)
> + # cwd = os.getcwd()
> + os.symlink(os.path.join(self.image.base, f),
> + os.path.join(self.stagedir, f))
> + print "Writing %s" % outfile
> + cmd = "tar chzv -C %s -f %s %s" % (self.tmpdir,
> + outfile,
> + os.path.basename(self.vername))
> + util.system(cmd)
> + return outfile
I missed this above message first time around: might want to also wrap
"Writing %s" to make it translatable.
> +
> +### Option parsing
> +def parse_args():
> + parser = OptionParser()
> + parser.set_usage("%prog [options] image.xml")
> +
> + parser.add_option("-o", "--output", type="string", dest="output",
> + action="callback", callback=cli.check_before_store,
> + help=_("Directory in which packaged file will be put"))
> + parser.add_option("-d", "--debug", action="store_true", dest="debug",
> + help=_("Print debugging information"))
> +
> + (options,args) = parser.parse_args()
> + if len(args) < 1:
> + parser.error(_("You need to provide an image XML descriptor"))
> + options.image = args[0]
> +
> + return options
> +
> +def main():
> + options = parse_args()
> + cli.setupLogging("virt-pack", options.debug)
> + image = virtinst.ImageParser.parse_file(os.path.abspath(options.image))
> + if image.name is None or image.version is None:
> + print >> sys.stderr, _("The image descriptor must contain name and version")
> + valid = False
> +
> + if options.output is None:
> + options.output = os.path.join(image.base, "..")
> +
> + pkg = Package(image)
> + try:
> + pkg.add_image_files()
> + except PackageException, e:
> + print >> sys.stderr, _("Validation failed: %s"), e
> + return 1
> +
> + try:
> + pkg.make_vmx_files()
> + pkg.pack(options.output)
> + except PackageException, e:
> + print >> sys.stderr, _("Packaging failed: %s") % e
> + return 1
> +
> +if __name__ == "__main__":
> + main()
> +
Thanks,
Cole
_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/et-mgmt-tools
