Hey Rodrigo. On Wed, 2015-09-16 at 13:06 -0300, Rodrigo Trujillo wrote:

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 ?

First of all, this patch set solves an odd issue that happens only with RAW images. So, for all the other types of images (qcow, vdms, etc), it assumes everything is operating as expected. Regarding the 'magic' (file command's magic pattern file) list, those two items I checked on x86_64 and Power machines when I executed my tests. Now, after you asked about them, I checked again in a few more machines and found one more supported magic that is valid to RAW images. So I must send a V2 adding the new one.

2- Could ['dos/mbr boot sector', 'data'] be defined as global/constant ?

Yes, it is possible. However this list is used only in this particular block and, IMO, I don't think necessary do that. This is not a big issue, so I can change this in a V2 :-D

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.

The check happens only for RAW volumes - if the volume is not a RAW volume, the code assumes it's a valid volume. In addition, the 'isvalid' information is being returned in the GET action when asked by volume information. The UI already has received the list with the information of all volumes for a given storage pool. The only modification was add a new information field in the dictionary that returns the information of the volume.

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