[Kimchi-devel] [PATCH 2/3] Raw volumes validation: back-end and front-end
Paulo Ricardo Paz Vital
pvital at linux.vnet.ibm.com
Wed Sep 16 16:47:26 UTC 2015
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 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
>
More information about the Kimchi-devel
mailing list