On 04/04/2016 10:01 PM, Jose Ricardo Ziviani wrote:

Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- vmtemplate.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/vmtemplate.py b/vmtemplate.py index 653ad02..b610d0c 100644 --- a/vmtemplate.py +++ b/vmtemplate.py @@ -96,7 +96,11 @@ class VMTemplate(object): for index, disk in enumerate(disks): disk_info = dict(default_disk)

- pool_type = self._get_storage_type(disk['pool']['name']) + pool_name = '/plugins/kimchi/storagepools/default'

Why are you assuming this pool as default? The default pool come from osinfo and may be changed by user according to template.conf file, so assuming that may cause problems in case user changed the default pool in some way.

+ if 'pool' in disk and 'name' in disk['pool']: + pool_name = disk['pool']['name'] + + pool_type = self._get_storage_type(pool_name) if pool_type in ['iscsi', 'scsi']: disk_info = {'index': 0, 'format': 'raw', 'volume': None}

On 04/04/2016 10:02 PM, Jose Ricardo Ziviani wrote:

Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- model/templates.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/model/templates.py b/model/templates.py index 92705b6..2a722b9 100644 --- a/model/templates.py +++ b/model/templates.py @@ -271,7 +271,7 @@ class LibvirtVMTemplate(VMTemplate): # validate CPU info values - will raise appropriate exceptions cpu_model.check_cpu_info(self.info['cpu_info'])

- def _storage_validate(self, pool_uri): + def _storage_validate(self, pool_uri, throw_if_not_active=True): pool_name = pool_name_from_uri(pool_uri) try: conn = self.conn.get() @@ -280,7 +280,11 @@ class LibvirtVMTemplate(VMTemplate): raise InvalidParameter("KCHTMPL0004E", {'pool': pool_uri, 'template': self.name})

- if not pool.isActive():

+ # TODO: refactor this code because an inactive pool is not an + # exception, it is a valid and expect flow. It should be the + # called responsibility to decide what to do when the pool + # is not active. + if not pool.isActive() and throw_if_not_active: raise InvalidParameter("KCHTMPL0005E", {'pool': pool_name, 'template': self.name})

In this case, the pool should be add to the 'invalid' parameter to let user knows the Template is invalid. We have time to do it for 2.2, so I'd appreciate if you could do the right solution now =)

@@ -315,7 +319,7 @@ class LibvirtVMTemplate(VMTemplate): return xpath_get_text(xml, "/pool/target/path")[0]

def _get_storage_type(self, pool_uri=None): - pool = self._storage_validate(pool_uri) + pool = self._storage_validate(pool_uri, False) xml = pool.XMLDesc(0) return xpath_get_text(xml, "/pool/@type")[0]

I am not sure if I understood the idea of this patch. From the bug description, I saw a problem when editing a Template instead when creating a new one. But per this patch, you modified the Template creation process. In the update process, we just need to validate the data and raise an error if any exists and do not delete anything. So when removing an ISO from system and try to use this same path in an update process, the error should be raised to inform user about the invalid input and it is all. When canceling the operation, no request should be sent to server. Did I misunderstand the problem or could you explain better what do you have in mind to solve the bug? On 04/04/2016 10:02 PM, Jose Ricardo Ziviani wrote:

Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- model/templates.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/model/templates.py b/model/templates.py index 2a722b9..1b2be0e 100644 --- a/model/templates.py +++ b/model/templates.py @@ -51,7 +51,11 @@ class TemplatesModel(object):

def create(self, params): name = params.get('name', '').strip() - iso = params.get('cdrom') + if params.get('force_no_cdrom', None) is None: + iso = None + else: + iso = params.get('cdrom') + # check search permission if iso and iso.startswith('/') and os.path.exists(iso): st_mode = os.stat(iso).st_mode @@ -74,7 +78,10 @@ class TemplatesModel(object): # Creates the template class with necessary information # Checkings will be done while creating this class, so any exception # will be raised here - t = LibvirtVMTemplate(params, scan=True, conn=self.conn) + scan = False + if params.get('force_no_cdrom', None) is None: + scan = True + t = LibvirtVMTemplate(params, scan=scan, conn=self.conn)

# Validate cpu info t.cpuinfo_validate() @@ -206,6 +213,13 @@ class TemplateModel(object): 'template': name})

self.delete(name) + + # if the cdrom is invalid (if it was removed) the template will be + # deleted but never replaced back, users won't understand why because + # it's not clear. In this sceario, since a are replacing an existing + # temaplte, let's create it anyway and gives the user a chance to + # fix the cdrom name. + new_t['force_no_cdrom'] = True try: ident = self.templates.create(new_t) except: