[Libstoragemgmt-devel] [PATCH 4/6] C & PY: Public function/method to check vpd format

[Tony Asleson]

The library expects and now enforces 32 character lower case hex
representation for vpd 0x83.

Added:

C: lsm_volume_vpd83_verify
Py: Volume.vpd83_verify

To allow a user the ability to validate the vpd.

Signed-off-by: Tony Asleson <***@redhat.com>
---
.../include/libstoragemgmt/libstoragemgmt_common.h | 8 +++++++
c_binding/lsm_datatypes.cpp | 15 +++++++++---
c_binding/lsm_mgmt.cpp | 18 +++++++++++++++
python_binding/lsm/_data.py | 21 +++++++++++++----
test/plugin_test.py | 14 +++++++++++
test/tester.c | 27 ++++++++++++++++++++++
6 files changed, 96 insertions(+), 7 deletions(-)

diff --git a/c_binding/include/libstoragemgmt/libstoragemgmt_common.h b/c_binding/include/libstoragemgmt/libstoragemgmt_common.h
index 360b49a..64404bb 100644
--- a/c_binding/include/libstoragemgmt/libstoragemgmt_common.h
+++ b/c_binding/include/libstoragemgmt/libstoragemgmt_common.h
@@ -121,6 +121,14 @@ int LSM_DLL_EXPORT lsm_string_list_delete(lsm_string_list *sl, uint32_t index);
int LSM_DLL_EXPORT lsm_initiator_id_verify( const char *init_id,
lsm_access_group_init_type *init_type);

+
+/**
+ * Checks to see if volume vpd83 is valid
+ * @param vpd83 VPD string to check
+ * @return LSM_ERR_OK if vpd is OK, else LSM_INVALID_ARGUMENT
+ */
+int LSM_DLL_EXPORT lsm_volume_vpd83_verify( const char *vpd83 );
+
#ifdef __cplusplus
}
#endif
diff --git a/c_binding/lsm_datatypes.cpp b/c_binding/lsm_datatypes.cpp
index c8475e6..4826a6b 100644
--- a/c_binding/lsm_datatypes.cpp
+++ b/c_binding/lsm_datatypes.cpp
@@ -644,14 +644,23 @@ CREATE_ALLOC_ARRAY_FUNC(lsm_volume_record_array_alloc, lsm_volume *)
lsm_volume * lsm_volume_record_alloc(const char *id, const char *name,
const char *vpd83, uint64_t blockSize,
uint64_t numberOfBlocks,
- uint32_t status, const char *system_id, const char *pool_id, const char* plugin_data)
+ uint32_t status, const char *system_id, const char *pool_id,
+ const char* plugin_data)
{
+ if( vpd83 && (LSM_ERR_OK != lsm_volume_vpd83_verify(vpd83)) ) {
+ return NULL;
+ }
+
lsm_volume *rc = (lsm_volume *)calloc(1, sizeof(lsm_volume));
if (rc) {
rc->magic = LSM_VOL_MAGIC;
rc->id = strdup(id);
rc->name = strdup(name);
- rc->vpd83 = strdup(vpd83);
+
+ if( vpd83 ) {
+ rc->vpd83 = strdup(vpd83);
+ }
+
rc->block_size = blockSize;
rc->number_of_blocks = numberOfBlocks;
rc->admin_state = status;
@@ -662,7 +671,7 @@ lsm_volume * lsm_volume_record_alloc(const char *id, const char *name,
rc->plugin_data = strdup(plugin_data);
}

- if( !rc->id || !rc->name || !rc->vpd83 || !rc->system_id ||
+ if( !rc->id || !rc->name || (vpd83 && !rc->vpd83) || !rc->system_id ||
!rc->pool_id ||
(plugin_data && !rc->plugin_data)) {
lsm_volume_record_free(rc);
diff --git a/c_binding/lsm_mgmt.cpp b/c_binding/lsm_mgmt.cpp
index 36f616d..37faed4 100644
--- a/c_binding/lsm_mgmt.cpp
+++ b/c_binding/lsm_mgmt.cpp
@@ -113,6 +113,24 @@ int lsm_initiator_id_verify(const char *init_id,
return rc;
}

+int lsm_volume_vpd83_verify( const char *vpd83 )
+{
+ int rc = LSM_ERR_INVALID_ARGUMENT;
+ int i;
+
+ if( vpd83 && strlen(vpd83) == 32 ) {
+ for(i = 0; i < 32; ++i) {
+ char v = vpd83[i];
+ // 0-9 || a-f is OK
+ if( !((v >= 48 && v <= 57) || (v >= 97 && v <= 102)) ) {
+ return rc;
+ }
+ }
+ rc = LSM_ERR_OK;
+ }
+ return rc;
+}
+
static int verify_initiator_id(const char *id, lsm_access_group_init_type t,
Value &initiator)
{
diff --git a/python_binding/lsm/_data.py b/python_binding/lsm/_data.py
index 41597d1..2fb1d1f 100644
--- a/python_binding/lsm/_data.py
+++ b/python_binding/lsm/_data.py
@@ -222,6 +222,11 @@ class Disk(IData):
return self.name


+# Lets do this once outside of the class to minimize the number of
+# times it needs to be compiled.
+_vol_regex_vpd83 = re.compile('^[0-9a-f]{32}$')
+
+
@default_property('id', doc="Unique identifier")
@default_property('name', doc="User given name")
@default_property('vpd83', doc="Vital product page 0x83 identifier")
@@ -254,15 +259,14 @@ class Volume(IData):
ADMIN_STATE_DISABLED = 0
ADMIN_STATE_ENABLED = 1

- _regex_vpd83_str = re.compile(r"""^[0-9a-f]{32}$""")
-
def __init__(self, _id, _name, _vpd83, _block_size, _num_of_blocks,
_admin_state, _system_id, _pool_id, _plugin_data=None):
self._id = _id # Identifier
self._name = _name # Human recognisable name
- if _vpd83 and not Volume._regex_vpd83_str.match(_vpd83):
+ if _vpd83 and not Volume.vpd83_verify(_vpd83):
raise LsmError(ErrorNumber.INVALID_ARGUMENT,
- "Incorrect format of VPD 0x83 string: '%s'" %
+ "Incorrect format of VPD 0x83 string: '%s', "
+ "expecting 32 lower case hex characters" %
_vpd83)
self._vpd83 = _vpd83 # SCSI page 83 unique ID
self._block_size = _block_size # Block size
@@ -282,6 +286,15 @@ class Volume(IData):
def __str__(self):
return self.name

+ @staticmethod
+ def vpd83_verify(vpd):
+ """
+ Returns True if string is valid vpd 0x83 representation
+ """
+ if vpd and _vol_regex_vpd83.match(vpd):
+ return True
+ return False
+

@default_property('id', doc="Unique identifier")
@default_property('name', doc="User defined system name")
diff --git a/test/plugin_test.py b/test/plugin_test.py
index 8679e54..eb09b23 100755
--- a/test/plugin_test.py
+++ b/test/plugin_test.py
@@ -975,6 +975,20 @@ class TestPlugin(unittest.TestCase):

self.c.access_group_delete(ag)

+ def test_volume_vpd83_verify(self):
+
+ failing = [None,
+ "012345678901234567890123456789AB",
+ "012345678901234567890123456789ax",
+ "012345678901234567890123456789ag",
+ "1234567890123456789012345abcdef",
+ "01234567890123456789012345abcdefa"]
+
+ for f in failing:
+ self.assertFalse(lsm.Volume.vpd83_verify(f))
+
+ self.assertTrue(
+ lsm.Volume.vpd83_verify("01234567890123456789012345abcdef"))


def dump_results():
diff --git a/test/tester.c b/test/tester.c
index 01a8513..4cf2f28 100644
--- a/test/tester.c
+++ b/test/tester.c
@@ -66,6 +66,17 @@ fail_unless( LSM_ERR_OK == variable, "call:%s rc = %d %s (which %d)", #func, \
variable, error(lsm_error_last_get(c)), which_plugin);

/**
+ * Macro for calls which we expect failure.
+ * @param variable Where the result of the call is placed
+ * @param func Name of function
+ * @param ... Function parameters
+ */
+#define F(variable, func, ...) \
+variable = func(__VA_ARGS__); \
+fail_unless( LSM_ERR_OK != variable, "call:%s rc = %d %s (which %d)", #func, \
+ variable, error(lsm_error_last_get(c)), which_plugin);
+
+/**
* Generates a random string in the buffer with specified length.
* Note: This function should not produce repeating sequences or duplicates
* regardless if it's used repeatedly in the same function in the same process
@@ -2827,6 +2838,21 @@ START_TEST(test_initiator_id_verification)
}
END_TEST

+START_TEST(test_volume_vpd_check)
+{
+ int rc;
+
+ F(rc, lsm_volume_vpd83_verify, NULL );
+ F(rc, lsm_volume_vpd83_verify, "012345678901234567890123456789AB");
+ F(rc, lsm_volume_vpd83_verify, "012345678901234567890123456789ax");
+ F(rc, lsm_volume_vpd83_verify, "012345678901234567890123456789ag");
+ F(rc, lsm_volume_vpd83_verify, "1234567890123456789012345abcdef");
+ F(rc, lsm_volume_vpd83_verify, "01234567890123456789012345abcdefa");
+
+ G(rc, lsm_volume_vpd83_verify, "01234567890123456789012345abcdef");
+}
+END_TEST
+
Suite * lsm_suite(void)
{
Suite *s = suite_create("libStorageMgmt");
@@ -2834,6 +2860,7 @@ Suite * lsm_suite(void)
TCase *basic = tcase_create("Basic");
tcase_add_checked_fixture (basic, setup, teardown);

+ tcase_add_test(basic, test_volume_vpd_check);
tcase_add_test(basic, test_initiator_id_verification);
tcase_add_test(basic, test_target_ports);
tcase_add_test(basic, test_search_fs);

[Tony Asleson]

From: Gris Ge <***@redhat.com>

* Updated these plugins for Volume.vpd83 property:
SMI-S
Nstor
sim

* These plugins already set correct vpd83:
simc
ontap

V2: Fix merge conflicts
Simplify regex. string in smis.py

Signed-off-by: Gris Ge <***@redhat.com>
Signed-off-by: Tony Asleson <***@redhat.com>
---
plugin/nstor/nstor.py | 5 +++--
plugin/sim/simarray.py | 4 ++--
plugin/smispy/smis.py | 5 ++++-
3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/plugin/nstor/nstor.py b/plugin/nstor/nstor.py
index 4446972..bcb948f 100644
--- a/plugin/nstor/nstor.py
+++ b/plugin/nstor/nstor.py
@@ -466,8 +466,9 @@ class NexentaStor(INfs, IStorageAreaNetwork):
admin_state = Volume.ADMIN_STATE_ENABLED

vol_list.append(
- Volume(lu, lu, lu_props['guid'], block_size, num_of_blocks,
- admin_state, self._system.id,
+ Volume(lu, lu, lu_props['guid'].lower(),
+ block_size, num_of_blocks,
+ admin_state, self.system.id,
NexentaStor._get_pool_id(lu)))

return search_property(vol_list, search_key, search_value)
diff --git a/plugin/sim/simarray.py b/plugin/sim/simarray.py
index 038d2a4..6e039c5 100644
--- a/plugin/sim/simarray.py
+++ b/plugin/sim/simarray.py
@@ -543,7 +543,7 @@ class SimData(object):
}
"""
SIM_DATA_BLK_SIZE = 512
- SIM_DATA_VERSION = "2.8"
+ SIM_DATA_VERSION = "2.9"
SIM_DATA_SYS_ID = 'sim-01'
SIM_DATA_INIT_NAME = 'NULL'
SIM_DATA_TMO = 30000 # ms
@@ -786,7 +786,7 @@ class SimData(object):
"""
vpd = []
for i in range(0, l):
- vpd.append(str('%02X' % (random.randint(0, 255))))
+ vpd.append(str('%02x' % (random.randint(0, 255))))
return "".join(vpd)

def _size_of_raid(self, member_type, member_ids, raid_type,
diff --git a/plugin/smispy/smis.py b/plugin/smispy/smis.py
index 36eabf5..14f1ce9 100644
--- a/plugin/smispy/smis.py
+++ b/plugin/smispy/smis.py
@@ -24,6 +24,7 @@ import copy
import os
import datetime
import sys
+import re

import pywbem
from pywbem import CIMError
@@ -1267,7 +1268,9 @@ class Smis(IStorageAreaNetwork):
if vpd_83 is None:
vpd_83 = Smis._vpd83_in_cv_ibm_xiv(cv)

- if vpd_83 is None:
+ if vpd_83 and re.match('^[a-fA-F0-9]{32}$', vpd_83):
+ vpd_83 = vpd_83.lower()
+ else:
vpd_83 = ''

#This is a fairly expensive operation, so it's in our best interest

[Tony Asleson]

Signed-off-by: Tony Asleson <***@redhat.com>
---
test/Makefile.am | 2 +-
test/runtests.sh | 6 ++++++
2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/test/Makefile.am b/test/Makefile.am
index a5eb3c5..872e8e0 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -3,7 +3,7 @@ AM_CPPFLAGS = \
-***@srcdir@/c_binding/include \
$(LIBXML_CFLAGS)

-EXTRA_DIST=cmdtest.py runtests.sh
+EXTRA_DIST=cmdtest.py runtests.sh plugin_test.py

TESTS = runtests.sh

diff --git a/test/runtests.sh b/test/runtests.sh
index a2840e9..ed7f19c 100755
--- a/test/runtests.sh
+++ b/test/runtests.sh
@@ -159,7 +159,13 @@ if [ -z "$LSM_VALGRIND" ]; then
export LSMCLI_URI='sim://'
good "$rootdir/test/cmdtest.py -c $plugins/sim_lsmplugin"
good "$rootdir/test/cmdtest.py -c $rootdir/tools/lsmcli/lsmcli"
+
+ #Run the plug-in test against the python simulator
+ good "$rootdir/test/plugin_test.py -v --uri sim://"
fi

+#Run the plug-in test against the C simulator"
+good "$rootdir/test/plugin_test.py -v --uri simc://"
+
#Pretend we were never here
cleanup

[Tony Asleson]

Signed-off-by: Tony Asleson <***@redhat.com>
---
test/plugin_test.py | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/test/plugin_test.py b/test/plugin_test.py
index 2e1f295..8679e54 100755
--- a/test/plugin_test.py
+++ b/test/plugin_test.py
@@ -308,9 +308,7 @@ class TestPlugin(unittest.TestCase):

@staticmethod
def _vpd_correct(vpd):
- p = re.compile('^[a-fA-F0-9]+$')
-
- if vpd is not None and len(vpd) > 0 and p.match(vpd) is not None:
+ if vpd and re.match('^[a-f0-9]{32}$', vpd):
return True
return False

[Tony Asleson]

From: Gris Ge <***@redhat.com>

* Raise error(INVALID_ARGUMENT) if got incorrect format of vpd83, should be
lower 32 hex digitals.

Signed-off-by: Gris Ge <***@redhat.com>
---
python_binding/lsm/_data.py | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/python_binding/lsm/_data.py b/python_binding/lsm/_data.py
index 4031936..41597d1 100644
--- a/python_binding/lsm/_data.py
+++ b/python_binding/lsm/_data.py
@@ -254,10 +254,16 @@ class Volume(IData):
ADMIN_STATE_DISABLED = 0
ADMIN_STATE_ENABLED = 1

+ _regex_vpd83_str = re.compile(r"""^[0-9a-f]{32}$""")
+
def __init__(self, _id, _name, _vpd83, _block_size, _num_of_blocks,
_admin_state, _system_id, _pool_id, _plugin_data=None):
self._id = _id # Identifier
self._name = _name # Human recognisable name
+ if _vpd83 and not Volume._regex_vpd83_str.match(_vpd83):
+ raise LsmError(ErrorNumber.INVALID_ARGUMENT,
+ "Incorrect format of VPD 0x83 string: '%s'" %
+ _vpd83)
self._vpd83 = _vpd83 # SCSI page 83 unique ID
self._block_size = _block_size # Block size
self._num_of_blocks = _num_of_blocks # Number of blocks

[Tony Asleson]

Change this so random names are in the form:

lsm_<component>_<random characters>

Signed-off-by: Tony Asleson <***@redhat.com>
---
test/plugin_test.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/plugin_test.py b/test/plugin_test.py
index eb09b23..f84e2dd 100755
--- a/test/plugin_test.py
+++ b/test/plugin_test.py
@@ -74,7 +74,7 @@ def rs(component, l=4):
rp = ''.join(random.choice(string.ascii_uppercase) for x in range(l))

if component is not None:
- return 'lsm_%s_' % (component + rp)
+ return 'lsm_%s_%s' % (component, rp)
return rp

[Gris Ge]

Gris had some patches that started with this so I used what was
available and added some C bits and made the checks public so
that developers can check the validity of the vpd before calling
into the library with it.
This patch set also includes running the plug-in test against the
C and PY simulators.

Hi Tony,

Looks good to me. Thanks for provide V2 for my previous patch.

Just found one issue: PyYAML should be add into autoconf and rpm SPEC
for 'make check' plugin_test.py seek. Check coming patch for detail.

Thank you.
Best regards.

[Gris Ge]

As plugin_test.py has been added into `make check`, we need to make sure
PyYAML is installed by RPM SPEC or autoconf.

Signed-off-by: Gris Ge <***@redhat.com>
---
configure.ac | 1 +
packaging/libstoragemgmt.spec.in | 1 +
2 files changed, 2 insertions(+)

diff --git a/configure.ac b/configure.ac
index 92c164e..8d6c288 100644
--- a/configure.ac
+++ b/configure.ac
@@ -160,6 +160,7 @@ AM_PATH_PYTHON([2.6], [], AC_MSG_ERROR([Python interpreter 2.6 or 2.7 required])
AC_PYTHON_MODULE([pywbem], [Required])
AC_PYTHON_MODULE([M2Crypto], [Required])
AC_PYTHON_MODULE([argparse], [Required])
+AC_PYTHON_MODULE([yaml], [Required])

dnl ==========================================================================
dnl Check for libmicrohttpd and json-c as it is needed for REST API daemon
diff --git a/packaging/libstoragemgmt.spec.in b/packaging/libstoragemgmt.spec.in
index 22d1c20..a1d6b18 100644
--- a/packaging/libstoragemgmt.spec.in
+++ b/packaging/libstoragemgmt.spec.in
@@ -21,6 +21,7 @@ BuildRequires: python-argparse
BuildRequires: glib2-devel
# Explicitly require gcc-c++ is for OBS
BuildRequires: gcc-c++
+BuildRequires: PyYAML

# For OBS only. OBS is still using RHEL/Centos 6.2 which does not have
# OrderedDict from python collection. Use EPEL6 python-ordereddict.

[Gris Ge]

Post by Gris Ge
As plugin_test.py has been added into `make check`, we need to make sure
PyYAML is installed by RPM SPEC or autoconf.

Tested in OBS for RHEL 6/7 and Fedora 18/19/20.
https://build.opensuse.org/project/monitor/home:cathay4t:libstoragemgmt-test

[Tony Asleson]

Committed!

Thanks,
Tony

Post by Gris Ge
As plugin_test.py has been added into `make check`, we need to make sure
PyYAML is installed by RPM SPEC or autoconf.
---
configure.ac | 1 +
packaging/libstoragemgmt.spec.in | 1 +
2 files changed, 2 insertions(+)
diff --git a/configure.ac b/configure.ac
index 92c164e..8d6c288 100644
--- a/configure.ac
+++ b/configure.ac
@@ -160,6 +160,7 @@ AM_PATH_PYTHON([2.6], [], AC_MSG_ERROR([Python interpreter 2.6 or 2.7 required])
AC_PYTHON_MODULE([pywbem], [Required])
AC_PYTHON_MODULE([M2Crypto], [Required])
AC_PYTHON_MODULE([argparse], [Required])
+AC_PYTHON_MODULE([yaml], [Required])
dnl ==========================================================================
dnl Check for libmicrohttpd and json-c as it is needed for REST API daemon
diff --git a/packaging/libstoragemgmt.spec.in b/packaging/libstoragemgmt.spec.in
index 22d1c20..a1d6b18 100644
--- a/packaging/libstoragemgmt.spec.in
+++ b/packaging/libstoragemgmt.spec.in
@@ -21,6 +21,7 @@ BuildRequires: python-argparse
BuildRequires: glib2-devel
# Explicitly require gcc-c++ is for OBS
BuildRequires: gcc-c++
+BuildRequires: PyYAML
# For OBS only. OBS is still using RHEL/Centos 6.2 which does not have
# OrderedDict from python collection. Use EPEL6 python-ordereddict.