On 04/06/2016 01:18 AM, Ramon Medeiros wrote:
First, improve functions inside class. This will improve code reading, also, make more functions to split procedures like: validating disks, graphics xml and distro checking.
Add separate function to identify the installation media.
Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- model/templates.py | 17 +++------ vmtemplate.py | 104 ++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 92 insertions(+), 29 deletions(-)
diff --git a/model/templates.py b/model/templates.py index 28c8a08..ca540cd 100644 --- a/model/templates.py +++ b/model/templates.py @@ -51,25 +51,12 @@ class TemplatesModel(object):
def create(self, params): name = params.get('name', '').strip() - iso = params.get('cdrom')
Join this patch with 2/4
# template with the same name already exists: raise exception with self.objstore as session: if name in session.get_list('template'): raise InvalidOperation("KCHTMPL0001E", {'name': name})
- # check search permission - if iso and iso.startswith('/') and os.path.exists(iso): - st_mode = os.stat(iso).st_mode - if stat.S_ISREG(st_mode) or stat.S_ISBLK(st_mode): - user = UserTests().probe_user() - run_setfacl_set_attr(iso, user=user) - ret, excp = probe_file_permission_as_user(iso, user) - if ret is False: - raise InvalidParameter('KCHISO0008E', - {'filename': iso, 'user': user, - 'err': excp}) - conn = self.conn.get() for net_name in params.get(u'networks', []): try: @@ -198,6 +185,10 @@ class TemplateModel(object): params['memory'].update(new_mem) validate_memory(params['memory'])
+ # params are cdrom or disk: add it to source_media + if "cdrom" in params: + params["source_media"] = params.get("cdrom") +
WHY? cdrom will not be on params! Now the only way to create a Template is through source_media!
new_t.update(params)
for net_name in params.get(u'networks', []): diff --git a/vmtemplate.py b/vmtemplate.py index 653ad02..7073d3a 100644 --- a/vmtemplate.py +++ b/vmtemplate.py @@ -17,6 +17,7 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+import magic import os import platform import stat @@ -40,6 +41,10 @@ from wok.plugins.kimchi.xmlutils.interface import get_iface_xml from wok.plugins.kimchi.xmlutils.qemucmdline import get_qemucmdline_xml from wok.plugins.kimchi.xmlutils.serial import get_serial_xml
+DISK_TYPE = {"QEMU QCOW Image": "qcow2", + "data": "raw"} +ISO_TYPE = "ISO 9660 CD-ROM" +
class VMTemplate(object): def __init__(self, args, scan=False): @@ -54,20 +59,13 @@ class VMTemplate(object): self.info = {} self.fc_host_support = args.get('fc_host_support')
- # Fetch defaults based on the os distro and version - try: - distro, version = self._get_os_info(args, scan) - except ImageFormatError as e: - raise OperationFailed('KCHTMPL0020E', {'err': e.message}) - os_distro = args.get('os_distro', distro) - os_version = args.get('os_version', version) - entry = osinfo.lookup(os_distro, os_version) - self.info.update(entry)
+ # no record found: create template + if scan:
The 'scan' is not related to new records! It is only to identify if the source_media will be scanned to get its OS and OS version. All the validation should continue to happen in all cases.
+ self._create_template(args, scan)
- # Auto-generate a template name if no one is passed - if 'name' not in args or args['name'] == '': - args['name'] = self._gen_name(distro, version) - self.name = args['name']
Why did you remove the above lines? Where the template name is generated if no one is passed?
+ # template already exists: load it + else: + self.info.update(args)
# Merge graphics settings graph_args = args.get('graphics') @@ -76,8 +74,6 @@ class VMTemplate(object): graphics.update(graph_args) args['graphics'] = graphics
- default_disk = self.info['disks'][0] - # Complete memory args, because dict method update is not recursive if 'memory' in args: if 'current' not in args['memory']: @@ -87,6 +83,42 @@ class VMTemplate(object):
# Override template values according to 'args' self.info.update(args) + + if self.info.get("disks") is not None and len(self.info["disks"]) >= 1: + default_disk = self.info['disks'][0] + + # validate disks + self._validate_disks(default_disk) + + def _create_template(self, args, scan=False): + """ + Creates a new template + """ + # no source_media argument: raise error + if args.get("source_media") is None: + raise MissingParameter('KCHTMPL0016E') + + # identify source media + self._identify_installation_media(args) + + # Fetch defaults based on the os distro and version + try: + distro, version = self._get_os_info(args, scan) + except ImageFormatError as e: + raise OperationFailed('KCHTMPL0020E', {'err': e.message}) + os_distro = args.get('os_distro', distro) + os_version = args.get('os_version', version) + entry = osinfo.lookup(os_distro, os_version) + + # update info + self.info.update(entry) + + # Auto-generate a template name if no one is passed + if 'name' not in args or args['name'] == '': + args['name'] = self._gen_name(distro, version) + self.name = args['name'] + + def _validate_disks(self, default_disk): disks = self.info.get('disks')
basic_disk = ['index', 'format', 'pool', 'size'] @@ -95,7 +127,6 @@ class VMTemplate(object):
for index, disk in enumerate(disks): disk_info = dict(default_disk) - pool_type = self._get_storage_type(disk['pool']['name']) if pool_type in ['iscsi', 'scsi']: disk_info = {'index': 0, 'format': 'raw', 'volume': None} @@ -118,6 +149,47 @@ class VMTemplate(object): disk_info['index'] = disk_info.get('index', index) self.info['disks'][index] = disk_info
+ def _identify_installation_media(self, args): + path = args.get("source_media") +
+ # user did not passed source media: return + if path is None: + return + + # create magic object to discover file type + file_type = magic.open(magic.MAGIC_NONE) + file_type.load() + + # file is local and exists: check if it's disk or cdrom + if path.startswith("/") and os.path.exists(path): + ftype = file_type.file(path)
It is better to check for http/https/tftp/ftp/ftps and already add it to cdrom before starting using magic. Example: if path.startswith(http/https/tftp/ftp/ftps): args[cdrom] = path return if not os.path.exists(path): raise InvalidParameter(Unable to find file %(path)s) # Otherwise, use magic to identify the file # create magic object to discover file type file_type = magic.open(magic.MAGIC_NONE) file_type.load() ftype = file_type.file(path)
+ + # cdrom + if ISO_TYPE in ftype: + args["cdrom"] = path + return +
# if it is not a cdrom, assume it is an image else: disks[0][base] = path
+ # disk + for types in DISK_TYPE.keys(): + if types in ftype: +
We don't need to do that as it is an easy point of failure due the amount of disk format we have.
+ # no disk list: create it + if args.get("disks") is None: + args["disks"] = [] +
+ osinfo_disk = osinfo.lookup("unknow", "unknow")["disks"] + + # os_info has default pool: get it + if osinfo_disk is not None: + args["disks"] = osinfo_disk +
Do not do this! The osinfo information will be get in other place. Only set disk[base] value as you did below.
+ args["disks"][0]["base"] = path
+ + # not local image: set as remote ISO + elif check_url_path(path): + args["cdrom"] = path + return +
isoinfo will do that verification! Here you can check the path starts with some patterns as I described below.
def _get_os_info(self, args, scan): distro = version = 'unknown'