As we have talked offline, we will keep using this check but I agree it needs to be improved by calling setfacl before checking the file permissions. On 18/03/2015 23:51, Crístian Viana wrote:

In some cases, the permission check was not allowing access to an image when the image was perfectly readable by libvirt.

Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 5 --- src/kimchi/kvmusertests.py | 75 ------------------------------------------- src/kimchi/model/templates.py | 12 ------- 3 files changed, 92 deletions(-) delete mode 100644 src/kimchi/kvmusertests.py

diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index df5422f..a43c72d 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -62,11 +62,6 @@ messages = { "KCHISO0005E": _("Invalid El Torito boot indicator in ISO %(filename)s"), "KCHISO0006E": _("Unexpected volume type for primary volume in ISO %(filename)s"), "KCHISO0007E": _("Bad format while reading volume descriptor in ISO %(filename)s"), - "KCHISO0008E": _("The hypervisor doesn't have permission to use this ISO %(filename)s. " - "Consider moving it under /var/lib/libvirt, or set the search permission " - "to file access control lists for '%(user)s' user if possible, or add the " - "'%(user)s' to the ISO path group, or (not recommended) 'chmod -R o+x 'path_to_iso'." - "Details: %(err)s" ),

"KCHIMG0001E": _("An error occurred when probing image OS information."), "KCHIMG0002E": _("No OS information found in given image."), diff --git a/src/kimchi/kvmusertests.py b/src/kimchi/kvmusertests.py deleted file mode 100644 index 37a80d7..0000000 --- a/src/kimchi/kvmusertests.py +++ /dev/null @@ -1,75 +0,0 @@ -# Project Kimchi -# -# Copyright IBM, Corp. 2014-2015 -# -# This library is free software; you can redistribute it and/or -# modify it under the terms of the GNU Lesser General Public -# License as published by the Free Software Foundation; either -# version 2.1 of the License, or (at your option) any later version. -# -# This library 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 -# Lesser General Public License for more details. -# -# You should have received a copy of the GNU Lesser General Public -# 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 platform -import psutil - -import libvirt - -from kimchi.rollbackcontext import RollbackContext - -KVMUSERTEST_VM_NAME = "KVMUSERTEST_VM" - - -class UserTests(object): - SIMPLE_VM_XML = """ - <domain type='kvm'> - <name>%(name)s</name> - <memory unit='KiB'>262144</memory> - <os> - <type arch='%(arch)s'>hvm</type> - <boot dev='hd'/> - </os> - </domain>""" - user = None - - @classmethod - def probe_user(cls): - if cls.user: - return cls.user - - arch = 'ppc64' if platform.machine() == 'ppc64le' \ - else platform.machine() - - xml = cls.SIMPLE_VM_XML % {'name': KVMUSERTEST_VM_NAME, 'arch': arch} - - with RollbackContext() as rollback: - conn = libvirt.open(None) - rollback.prependDefer(conn.close) - dom = conn.createXML(xml, - flags=libvirt.VIR_DOMAIN_START_AUTODESTROY) - rollback.prependDefer(dom.destroy) - filename = '/var/run/libvirt/qemu/%s.pid' % KVMUSERTEST_VM_NAME - with open(filename) as f: - pidStr = f.read() - p = psutil.Process(int(pidStr)) - - # bug fix #357 - # in psutil 2.0 and above versions, username will be a method, - # not a string - if callable(p.username): - cls.user = p.username() - else: - cls.user = p.username - - return cls.user - - -if __name__ == '__main__': - ut = UserTests() - print ut.probe_user() diff --git a/src/kimchi/model/templates.py b/src/kimchi/model/templates.py index e91636b..df0b551 100644 --- a/src/kimchi/model/templates.py +++ b/src/kimchi/model/templates.py @@ -18,16 +18,13 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

import copy -import os

import libvirt

from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import NotFoundError, OperationFailed -from kimchi.kvmusertests import UserTests from kimchi.model.cpuinfo import CPUInfoModel from kimchi.utils import pool_name_from_uri -from kimchi.utils import probe_file_permission_as_user from kimchi.vmtemplate import VMTemplate from kimchi.xmlutils.utils import xpath_get_text

@@ -39,15 +36,6 @@ class TemplatesModel(object):

def create(self, params): name = params.get('name', '').strip() - iso = params.get('cdrom') - # check search permission - if iso and iso.startswith('/') and os.path.isfile(iso): - user = UserTests().probe_user() - ret, excp = probe_file_permission_as_user(iso, user) - if ret is False: - raise InvalidParameter('KCHISO0008E', - {'filename': iso, 'user': user, - 'err': excp})

cpu_info = params.get('cpu_info') if cpu_info:

Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 18/03/2015 23:51, Crístian Viana wrote:

The current template code checks whether an ISO path is a regular file (i.e. not a directory, not a char device, not a link). However, a block device may also be a valid ISO path because storage volumes belonging to logical pools are always block devices (e.g. /dev/mypool/myvol.iso), and they should also be used to create templates.

Instead of allowing only regular files to be used as template ISOs, allow block devices as well.

Fix issue #565 (Allow creating templates with device files).

Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/isoinfo.py | 7 +++++-- src/kimchi/vmtemplate.py | 10 ++++++++-- 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py index b5a1769..badb002 100644 --- a/src/kimchi/isoinfo.py +++ b/src/kimchi/isoinfo.py @@ -22,6 +22,7 @@ import glob import platform import os import re +import stat import struct import sys import urllib2 @@ -151,8 +152,10 @@ class IsoImage(object): self._scan()

def _is_iso_remote(self): - if os.path.isfile(self.path): - return False + if os.path.exists(self.path): + st_mode = os.stat(self.path).st_mode + if stat.S_ISREG(st_mode) or stat.S_ISBLK(st_mode): + return False

if check_url_path(self.path): return True diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index ef41d0a..ec477dd 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -19,6 +19,7 @@

import os import socket +import stat import time import urlparse import uuid @@ -403,8 +404,13 @@ class VMTemplate(object): # validate iso integrity # FIXME when we support multiples cdrom devices iso = self.info.get('cdrom') - if iso and not (os.path.isfile(iso) or check_url_path(iso)): - invalid['cdrom'] = [iso] + if iso: + if os.path.exists(iso): + st_mode = os.stat(iso).st_mode + if not (stat.S_ISREG(st_mode) or stat.S_ISBLK(st_mode)): + invalid['cdrom'] = [iso] + elif not check_url_path(iso): + invalid['cdrom'] = [iso]

self.info['invalid'] = invalid

Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 18/03/2015 23:51, Crístian Viana wrote:

All storage volumes belonging to a logical storage pool are reported by libvirt as 'raw', even when they are ISO images. When creating a template, only the volumes listed as 'iso' are displayed, so it's currently not possible to create a template using a volume from a logical storage pool.

Instead of relying only on the volume format reported by libvirt, open and inspect the storage volumes which belong to a logical storage pool in order to find out whether they're ISO images.

Fix issue #564 (List volumes other than "format=iso" when creating a template).

Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 0480496..f02efca 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -294,6 +294,7 @@ class StorageVolumeModel(object): self.objstore = kargs['objstore'] self.task = TaskModel(**kargs) self.storagevolumes = StorageVolumesModel(**kargs) + self.storagepool = StoragePoolModel(**kargs)

@staticmethod def get_storagevolume(poolname, name, conn): @@ -321,6 +322,21 @@ class StorageVolumeModel(object): # infomation. When there is no format information, we assume # it's 'raw'. fmt = 'raw' + + iso_img = None + + # 'raw' volumes from 'logical' pools may actually be 'iso'; + # libvirt always reports them as 'raw' + pool_info = self.storagepool.lookup(pool) + if pool_info['type'] == 'logical' and fmt == 'raw': + try: + iso_img = IsoImage(path) + except IsoFormatError: + # not 'iso' afterall + pass + else: + fmt = 'iso' + ref_cnt = get_disk_ref_cnt(self.objstore, self.conn, path) res = dict(type=VOLUME_TYPE_MAP[info[0]], capacity=info[1], @@ -333,7 +349,8 @@ class StorageVolumeModel(object): path = os.path.join(os.path.dirname(path), os.readlink(path)) os_distro = os_version = 'unknown' try: - iso_img = IsoImage(path) + if iso_img is None: + iso_img = IsoImage(path) os_distro, os_version = iso_img.probe() bootable = True except IsoFormatError: