[Kimchi-devel] [PATCH 2/3] Raw volumes validation: back-end and front-end

[Rodrigo Trujillo]

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 at linux.vnet.ibm.com wrote:
> From: Paulo Vital <pvital at 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 at 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