Hi Paulo, I understood your approach and think that it works and will fix the issue. However, I would like to make some suggestions/questions: 1- Does ['dos/mbr boot sector', 'data'] work for all types of images supported by Kimchi ? 2- Could ['dos/mbr boot sector', 'data'] be defined as global/constant ? 3- Instead of return and check "isvalid" for each file/image inside the pool, I think a possible approach would be just don't return it in the API. Then the traffic and the UI checking are lesser. What do you think ? Rodrigo Trujillo On 09/15/2015 02:30 PM, pvital@linux.vnet.ibm.com wrote:
From: Paulo Vital <pvital@linux.vnet.ibm.com>
'Raw' volumes can not be valid image disks (e.g. XML, PDF, TXT are all raw files), so it's necessary check the 'content' of them and pass the information to front-end to make a better list of volume options on VM's edition screen.
This solves the issue #731.
Signed-off-by: Paulo Vital <pvital@linux.vnet.ibm.com> --- docs/API.md | 1 + src/kimchi/control/storagevolumes.py | 3 ++- src/kimchi/mockmodel.py | 6 ++++-- src/kimchi/model/storagevolumes.py | 15 ++++++++++++++- ui/js/src/kimchi.guest_storage_add.main.js | 2 +- 5 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/docs/API.md b/docs/API.md index 18d15ac..e94e69a 100644 --- a/docs/API.md +++ b/docs/API.md @@ -511,6 +511,7 @@ A interface represents available network interface on VM. * os_version *(optional)*: os version of the volume, for iso volume only. * bootable *(optional)*: True if iso image is bootable and not corrupted. * used_by: Name of vms which use this volume. + * isvalid: 1 if it's a valid volume, 0 otherwise.
* **DELETE**: Remove the Storage Volume * **POST**: *See Storage Volume Actions* diff --git a/src/kimchi/control/storagevolumes.py b/src/kimchi/control/storagevolumes.py index 8af7abc..d3532f6 100644 --- a/src/kimchi/control/storagevolumes.py +++ b/src/kimchi/control/storagevolumes.py @@ -57,7 +57,8 @@ class StorageVolume(Resource): 'allocation': self.info['allocation'], 'path': self.info['path'], 'used_by': self.info['used_by'], - 'format': self.info['format']} + 'format': self.info['format'], + 'isvalid': self.info['isvalid']}
for key in ('os_version', 'os_distro', 'bootable', 'base'): val = self.info.get(key) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 34bcfc9..33115ef 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -471,13 +471,15 @@ class MockStorageVolumes(object): 'allocation': 512, 'type': 'block', 'path': base_path + '1', - 'used_by': []}, + 'used_by': [], + 'isvalid': 1}, 'unit:0:0:2': {'capacity': 2048, 'format': 'unknown', 'allocation': 512, 'type': 'block', 'path': base_path + '2', - 'used_by': []}} + 'used_by': [], + 'isvalid': 1}}
class MockPartitions(object): diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 22856ff..27d4a4e 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -19,6 +19,7 @@
import contextlib import lxml.etree as ET +import magic import os import tempfile import threading @@ -310,13 +311,25 @@ class StorageVolumeModel(object): else: fmt = 'iso'
+ # 'raw' volumes can not be valid image disks (e.g. XML, PDF, TXT are + # raw files), so it's necessary check the 'content' of them + isvalid = 1 + valid_format_content = ['dos/mbr boot sector', 'data'] + if fmt == 'raw': + ms = magic.open(magic.NONE) + ms.load() + if ms.file(path).lower() not in valid_format_content: + isvalid = 0 + ms.close() + used_by = get_disk_used_by(self.objstore, self.conn, path) res = dict(type=VOLUME_TYPE_MAP[info[0]], capacity=info[1], allocation=info[2], path=path, used_by=used_by, - format=fmt) + format=fmt, + isvalid=isvalid) if fmt == 'iso': if os.path.islink(path): path = os.path.join(os.path.dirname(path), os.readlink(path)) diff --git a/ui/js/src/kimchi.guest_storage_add.main.js b/ui/js/src/kimchi.guest_storage_add.main.js index 3581c34..9a03656 100644 --- a/ui/js/src/kimchi.guest_storage_add.main.js +++ b/ui/js/src/kimchi.guest_storage_add.main.js @@ -81,7 +81,7 @@ kimchi.guest_storage_add_main = function() { if (result.length) { $.each(result, function(index, value) { // Only unused volume can be attached - if (value.used_by.length == 0 && (value.type != 'file' || validVolType[selectType].test(value.format))) { + if (value.used_by.length == 0 && value.isvalid == 1 && (value.type != 'file' || validVolType[selectType].test(value.format))) { options.push({ label: value.name, value: value.name