V2 lvm2app & python bindings bug fixes
cover V2 lvm2app & python bindings bug fixes
This patch set should resolve the following issues:
* Handle signed properties better
I added another flag on the properties structure to indicate if the property is signed and added a int64_t signed_integer to the union. Updated python binding to build the correct representation. I didn't get any feedback on this approach in the bz, not sure if another approach would work better.
ref. https://bugzilla.redhat.com/show_bug.cgi?id=838257
* Unable to retrieve specific properties:
Any property that internally was defined as a string list was broken, ended up being a missing flag.
ref. https://bugzilla.redhat.com/show_bug.cgi?id=1139920
Would appreciate someone taking a peek at these changes.
V2: Re-word the commit headers to provide more context.
Thanks,
Tony
Tony Asleson (5):
lvm2app: Add signed numerical property values
python: Build correct python value for numerical property
python: Check for NULL value before constructing string property
lvm2app: Correct missing string properties
Python: Improve lv property test coverage
lib/properties/prop_common.h | 5 +++- lib/report/columns.h | 8 +++---- lib/report/properties.c | 3 ++- lib/report/report.c | 2 ++ liblvm/lvm2app.h | 4 +++- liblvm/lvm_misc.c | 1 + liblvm/lvm_prop.c | 2 +- python/liblvm.c | 17 ++++++++++---- test/api/python_lvm_unit.py | 56 +++++++++++++++++++++++++++++++++++++++++--- 9 files changed, 83 insertions(+), 15 deletions(-)
Signature · 1 line
1.8.2.1
PATCH 1/5 lvm2app: Add signed numerical property values
Currently lvm2app properties have the following structure:
typedef struct lvm_property_value {
uint32_t is_settable:1;
uint32_t is_string:1;
uint32_t is_integer:1;
uint32_t is_valid:1;
uint32_t padding:28;
union {
const char *string;
uint64_t integer;
} value;
} lvm_property_value_t;which assumes that numerical values were in the range of 0 to 2**64-1. However, some of the properties were 'signed', like LV major/minor numbers and some reserved values for properties that represent percentages. Thus when the values were retrieved they were in two's complement notation. So for a -1 major number the API user would get a value of 18446744073709551615. The API user could cast the returned value to an int64_t to handle this, but that requires the API developer to look at the source code and determine when it should be done.
This change modifies the return property structure to:
typedef struct lvm_property_value {
uint32_t is_settable:1;
uint32_t is_string:1;
uint32_t is_integer:1;
uint32_t is_valid:1;
uint32_t is_signed:1;
uint32_t padding:27;
union {
const char *string;
uint64_t integer;
int64_t signed_integer;
} value;
} lvm_property_value_t;With this addition the API user can interrogate that the value is numerical, (is_integer = 1) and subsequently check if it's signed (is_signed = 1) too. If signed, then the API developer should use the union's signed_integer to avoid casting.
This change maintains backwards compatibility as the structure size remains unchanged and integer value remains unchanged. Only the additional bit taken from the pad is utilized.
Bugzilla reference:
https://bugzilla.redhat.com/show_bug.cgi?id=838257
--- lib/properties/prop_common.h | 5 ++++- lib/report/columns.h | 8 ++++---- lib/report/properties.c | 3 ++- lib/report/report.c | 2 ++ liblvm/lvm2app.h | 4 +++- liblvm/lvm_misc.c | 1 + liblvm/lvm_prop.c | 2 +- 7 files changed, 17 insertions(+), 8 deletions(-)
lib/properties/prop_common.h +4 -1
@@ -26,9 +26,11 @@ struct lvm_property_type { unsigned is_settable:1; unsigned is_string:1; unsigned is_integer:1; + unsigned is_signed:1;union {const char *string;uint64_t integer; +int64_t signed_integer; } value;int (*get) (const void *obj,struct lvm_property_type *prop);int (*set) (void *obj,struct lvm_property_type *prop);
@@ -126,9 +128,10 @@ static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \#define SIZ 4#define PCT 5#define STR_LIST 6 +#define SNUM 7/* Signed Number */ #define FIELD_MODIFIABLE 0x00000001#define FIELD (type, strct, field_type, head, field, width, fn, id, desc, settable) \ - { type, #id, settable, field_type == STR, ((field_type == NUM) || (field_type == BIN) || (field_type == SIZ) || (field_type == PCT)), { .integer = 0 }, _ ## id ## _get, _ ## id ## _set }, + { type, #id, settable, field_type == STR, ((field_type == NUM) || (field_type == BIN) || (field_type == SIZ) || (field_type == PCT) || (field_type == SNUM)), ((field_type == SNUM) || (field_type == PCT)), { .integer = 0 }, _ ## id ## _get, _ ## id ## _set },#endif
lib/report/columns.h +4 -4
@@ -56,8 +56,8 @@ FIELD(LVS, lv, STR, "Active", lvid, 6, lvactive, lv_active, "Active state of the FIELD(LVS, lv, BIN, "ActLocal", lvid, 10, lvactivelocally, lv_active_locally, "Set if the LV is active locally.", 0) FIELD(LVS, lv, BIN, "ActRemote", lvid, 10, lvactiveremotely, lv_active_remotely, "Set if the LV is active remotely.", 0) FIELD(LVS, lv, BIN, "ActExcl", lvid, 10, lvactiveexclusively, lv_active_exclusively, "Set if the LV is active exclusively.", 0) -FIELD(LVS, lv, NUM, "Maj", major, 3, int32, lv_major, "Persistent major number or -1 if not persistent.", 0) -FIELD(LVS, lv, NUM, "Min", minor, 3, int32, lv_minor, "Persistent minor number or -1 if not persistent.", 0) +FIELD(LVS, lv, SNUM, "Maj", major, 3, int32, lv_major, "Persistent major number or -1 if not persistent.", 0) +FIELD(LVS, lv, SNUM, "Min", minor, 3, int32, lv_minor, "Persistent minor number or -1 if not persistent.", 0) FIELD(LVS, lv, SIZ, "Rahead", lvid, 6, lvreadahead, lv_read_ahead, "Read ahead setting in current units.", 0) FIELD(LVS, lv, SIZ, "LSize", size, 5, size64, lv_size, "Size of LV in current units.", 0)FIELD (LVS , lv,SIZ ,"MSize" , lvid,6 , lvmetadatasize, lv_metadata_size,"For thin and cache pools, the size of the LV that holds the metadata." ,0 )
@@ -88,8 +88,8 @@ FIELD(LVS, lv, STR, "Time", lvid, 26, lvtime, lv_time, "Creation time of the LV, FIELD(LVS, lv, STR, "Host", lvid, 10, lvhost, lv_host, "Creation host of the LV, if known.", 0) FIELD(LVS, lv, STR_LIST, "Modules", lvid, 7, modules, lv_modules, "Kernel device-mapper modules required for this LV.", 0) -FIELD(LVSINFO, lv, NUM, "KMaj", lvid, 4, lvkmaj, lv_kernel_major, "Currently assigned major number or -1 if LV is not active.", 0) -FIELD(LVSINFO, lv, NUM, "KMin", lvid, 4, lvkmin, lv_kernel_minor, "Currently assigned minor number or -1 if LV is not active.", 0) +FIELD(LVSINFO, lv, SNUM, "KMaj", lvid, 4, lvkmaj, lv_kernel_major, "Currently assigned major number or -1 if LV is not active.", 0) +FIELD(LVSINFO, lv, SNUM, "KMin", lvid, 4, lvkmin, lv_kernel_minor, "Currently assigned minor number or -1 if LV is not active.", 0) FIELD(LVSINFO, lv, SIZ, "KRahead", lvid, 7, lvkreadahead, lv_kernel_read_ahead, "Currently-in-use read ahead setting in current units.", 0) FIELD(LVSINFO, lv, STR, "LPerms", lvid, 8, lvpermissions, lv_permissions, "LV permissions.", 0)FIELD (LVSINFO , lv,BIN ,"Suspended" , lvid,10 , lvsuspended, lv_suspended,"Set if LV is suspended." ,0 )
lib/report/properties.c +2 -1
@@ -462,7 +462,7 @@ GET_PVSEG_NUM_PROPERTY_FN(pvseg_size, (SECTOR_SIZE * pvseg->len))struct lvm_property_type _properties[] = { #include "columns.h" - {0 ,"" ,0 ,0 ,0 , { .integer =0 }, prop_not_implemented_get, prop_not_implemented_set }, + {0 ,"" ,0 ,0 ,0 ,0 , { .integer =0 }, prop_not_implemented_get, prop_not_implemented_set }, };#undef STR
@@ -471,6 +471,7 @@ struct lvm_property_type _properties[] = {#undef SIZ#undef PCT#undef STR_LIST +#undef SNUM#undef FIELD int lvseg_get_property(const struct lv_segment *lvseg,
lib/report/report.c +2 -0
@@ -2057,6 +2057,7 @@ static const struct dm_report_object_type _devtypes_report_types[] = {#define SIZ DM_REPORT_FIELD_TYPE_SIZE#define PCT DM_REPORT_FIELD_TYPE_PERCENT#define STR_LIST DM_REPORT_FIELD_TYPE_STRING_LIST +#define SNUM DM_REPORT_FIELD_TYPE_NUMBER#define FIELD (type, strct, sorttype, head, field, width, func, id, desc, writeable) \ {type, sorttype, offsetof(type_ ## strct, field), width, \ #id, head, &_ ## func ## _disp, desc},
@@ -2085,6 +2086,7 @@ static const struct dm_report_field_type _devtypes_fields[] = {#undef BIN#undef SIZ#undef STR_LIST +#undef SNUM#undef FIELD void *report_init(struct cmd_context *cmd, const char *format, const char *keys,
liblvm/lvm2app.h +3 -1
@@ -231,10 +231,12 @@ typedef struct lvm_property_value { uint32_t is_string:1; uint32_t is_integer:1; uint32_t is_valid:1; - uint32_t padding:28; + uint32_t is_signed:1; + uint32_t padding:27;union {const char *string;uint64_t integer; +int64_t signed_integer; } value; } lvm_property_value_t;
liblvm/lvm_misc.c +1 -0
@@ -88,6 +88,7 @@ struct lvm_property_value get_property(const pv_t pv, const vg_t vg, v.is_settable = prop.is_settable; v.is_string = prop.is_string; v.is_integer = prop.is_integer; + v.is_signed = prop.is_signed;if (v.is_string) v.value.string = prop.value.string;if (v.is_integer)
liblvm/lvm_prop.c +1 -1
@@ -41,7 +41,7 @@ SET_PVCREATEPARAMS_NUM_PROPERTY_FN(zero, pvcp->zero)struct lvm_property_type _lib_properties[] = { #include "lvm_prop_fields.h" - {0 ,"" ,0 ,0 ,0 , { .integer =0 }, prop_not_implemented_get, + {0 ,"" ,0 ,0 ,0 ,0 , { .integer =0 }, prop_not_implemented_get, prop_not_implemented_set }, };
Signature · 1 line
1.8.2.1
PATCH 1/5 lvm2app: Add signed numerical property values
Quoted 39 lines · Tony Asleson
Currently lvm2app properties have the following structure:
typedef struct lvm_property_value { uint32_t is_settable:1; uint32_t is_string:1; uint32_t is_integer:1; uint32_t is_valid:1; uint32_t padding:28; union { const char *string; uint64_t integer; } value; } lvm_property_value_t;which assumes that numerical values were in the range of 0 to 2**64-1. However, some of the properties were 'signed', like LV major/minor numbers and some reserved values for properties that represent percentages. Thus when the values were retrieved they were in two's complement notation. So for a -1 major number the API user would get a value of 18446744073709551615. The API user could cast the returned value to an
int64_tto handle this, but that requires the API developer to look at the source code and determine when it should be done.This change modifies the return property structure to:
typedef struct lvm_property_value { uint32_t is_settable:1; uint32_t is_string:1; uint32_t is_integer:1; uint32_t is_valid:1; uint32_t is_signed:1; uint32_t padding:27; union { const char *string; uint64_t integer; int64_t signed_integer; } value; } lvm_property_value_t;
...
Quoted 2 lines
lib/report/columns.h +2
-0+FIELD(LVS, lv, SNUM, "Maj", major, 3, int32, lv_major, "Persistent major number or -1 if not persistent.", 0) +FIELD (LVS , lv,SNUM ,"Min" , minor,3 , int32, lv_minor,"Persistent minor number or -1 if not persistent." ,0 )
...
Quoted 5 lines
lib/report/columns.h +2
-0+FIELD(LVSINFO, lv, SNUM, "KMaj", lvid, 4, lvkmaj, lv_kernel_major, "Currently assigned major number or -1 if LV is not active.", 0) +FIELD(LVSINFO, lv, SNUM, "KMin", lvid, 4, lvkmin, lv_kernel_minor, "Currently assigned minor number or -1 if LV is not active.", 0) FIELD(LVSINFO, lv, SIZ, "KRahead", lvid, 7, lvkreadahead, lv_kernel_read_ahead, "Currently-in-use read ahead setting in current units.", 0) FIELD(LVSINFO, lv, STR, "LPerms", lvid, 8, lvpermissions, lv_permissions, "LV permissions.", 0)FIELD (LVSINFO , lv,BIN ,"Suspended" , lvid,10 , lvsuspended, lv_suspended,"Set if LV is suspended." ,0 )
...
lib/report/report.c static const struct dm_report_object_type _devtypes_report_types[] = { · line 2060
Quoted 9 lines
lib/report/report.c +1
-0@@ -2057,6 +2057,7 @@ static const struct dm_report_object_type _devtypes_report_types[] = {#define SIZ DM_REPORT_FIELD_TYPE_SIZE#define PCT DM_REPORT_FIELD_TYPE_PERCENT#define STR_LIST DM_REPORT_FIELD_TYPE_STRING_LIST +#define SNUM DM_REPORT_FIELD_TYPE_NUMBER
Yes, for now, these negative numbers are here to indicate individual special values only (just like the "-1" for major/minor/kmajor/kminor when it's "undefined").
When it comes to reporting, we cover these extra values at the moment
with "reserved" values (defined in values.h). But once we need to report negative numbers fully, not just individual reserved values, we'd need similar patch for reporting part too, introducing new DM_REPORT_FIELD_TYPE_SIGNED_NUMBER.
But for now, I think it's fine to map SNUM to DM_REPORT_FILED_TYPE_NUMBER as we don't actually report negative numbers in their full range, it's just the "-1" (with the "undefined", "undef", "unknown" synonyms for those 4 fields).
But we surely need your patch for the liblvm as we don't export these synonyms/reserved values via liblvm in any way so liblvm users don't know about these mappings of these special values...
Signature · 1 line
Peter
PATCH 2/5 python: Build correct python value for numerical property
In reply to Tony AslesonWith the lvm2app C API adding the ability to determine when a property is signed we can then use this information to construct the correct representation of the number for python which will maintain value and sign. Previously, we only represented the numbers in python as positive integers.
Python long type exceeds the range for unsigned and signed integers, we just need to use the appropriate parsing code to build correctly.
Python part of the fix for:
https://bugzilla.redhat.com/show_bug.cgi?id=838257
--- python/liblvm.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
python/liblvm.c +8 -3
@@ -870,10 +870,15 @@ static PyObject *get_property(struct lvm_property_value *prop)if (!(pytuple =PyTuple_New (2 )))return NULL ; -if (prop->is_integer) -PyTuple_SET_ITEM (pytuple,0 ,Py_BuildValue ("K" , prop->value.integer)); -else +if (prop->is_integer) { +if (prop->is_signed) { +PyTuple_SET_ITEM (pytuple,0 ,Py_BuildValue ("L" , prop->value.signed_integer)); + }else { +PyTuple_SET_ITEM (pytuple,0 ,Py_BuildValue ("K" , prop->value.integer)); + } + }else {PyTuple_SET_ITEM (pytuple,0 ,PYSTRTYPE_FROMSTRING (prop->value.string)); + }if (prop->is_settable) setable = Py_True;
Signature · 1 line
1.8.2.1
PATCH 3/5 python: Check for NULL value before constructing string property
In reply to Tony AslesonWhen retrieving a property value that is a string, if the character pointer in C was NULL, we would segfault. This change checks for non-null before creating a python string representation. In the case where the character pointer is NULL we will return a python 'None' for the value.
--- python/liblvm.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
python/liblvm.c +5 -1
@@ -877,7 +877,11 @@ static PyObject *get_property(struct lvm_property_value *prop)PyTuple_SET_ITEM (pytuple,0 ,Py_BuildValue ("K" , prop->value.integer)); } }else { -PyTuple_SET_ITEM (pytuple,0 ,PYSTRTYPE_FROMSTRING (prop->value.string)); +if ( prop->value.string ) { +PyTuple_SET_ITEM (pytuple,0 ,PYSTRTYPE_FROMSTRING (prop->value.string)); + }else { +PyTuple_SET_ITEM (pytuple,0 , Py_None); + } }if (prop->is_settable)
Signature · 1 line
1.8.2.1
PATCH 4/5 lvm2app: Correct missing string properties
In reply to Tony AslesonSynopsis: STR_LIST needs to be treated as STR for properties.
For any lvm property that was internally 'typed' as a string list we were failing to return a string in the property API. This was due to the fact that for the properties to work the value needs to either be evaulated as a string or a number. This change corrects the macro used to build the memory array of structures so that the string bitfield is set as needed to ensure that the value is a string.
https://bugzilla.redhat.com/show_bug.cgi?id=1139920
--- lib/properties/prop_common.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/properties/prop_common.h +1 -1
@@ -132,6 +132,6 @@ static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \#define FIELD_MODIFIABLE 0x00000001#define FIELD (type, strct, field_type, head, field, width, fn, id, desc, settable) \ - { type, #id, settable, field_type == STR, ((field_type == NUM) || (field_type == BIN) || (field_type == SIZ) || (field_type == PCT) || (field_type == SNUM)), ((field_type == SNUM) || (field_type == PCT)), { .integer = 0 }, _ ## id ## _get, _ ## id ## _set }, + { type, #id, settable, (field_type == STR || field_type == STR_LIST), ((field_type == NUM) || (field_type == BIN) || (field_type == SIZ) || (field_type == PCT) || (field_type == SNUM)), ((field_type == SNUM) || (field_type == PCT)), { .integer = 0 }, _ ## id ## _get, _ ## id ## _set },#endif
Signature · 1 line
1.8.2.1
PATCH 4/5 lvm2app: Correct missing string properties
Quoted 21 lines · Tony Asleson
Synopsis:
STR_LISTneeds to be treated as STR for properties.For any lvm property that was internally 'typed' as a string list we were failing to return a string in the property API. This was due to the fact that for the properties to work the value needs to either be evaulated as a string or a number. This change corrects the macro used to build the memory array of structures so that the string bitfield is set as needed to ensure that the value is a string.
https://bugzilla.redhat.com/show_bug.cgi?id=1139920
Signed-off-by Tony Asleson <[email protected]>--- lib/properties/prop_common.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)lib/properties/prop_common.h +0
-0
lib/properties/prop_common.h static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \ · line 135
Quoted 5 lines
lib/properties/prop_common.h +1
-1@@ -132,6 +132,6 @@ static int _ ## NAME ## _get (const void *obj, struct lvm_property_type *prop) \#define FIELD_MODIFIABLE 0x00000001#define FIELD (type, strct, field_type, head, field, width, fn, id, desc, settable) \ - { type, #id, settable, field_type == STR, ((field_type == NUM) || (field_type == BIN) || (field_type == SIZ) || (field_type == PCT) || (field_type == SNUM)), ((field_type == SNUM) || (field_type == PCT)), { .integer = 0 }, _ ## id ## _get, _ ## id ## _set }, + { type, #id, settable, (field_type == STR || field_type == STR_LIST), ((field_type == NUM) || (field_type == BIN) || (field_type == SIZ) || (field_type == PCT) || (field_type == SNUM)), ((field_type == SNUM) || (field_type == PCT)), { .integer = 0 }, _ ## id ## _get, _ ## id ## _set },
My mistake! I always forget to add these. Thanks for fixing this!
Signature · 1 line
Peter
PATCH 5/5 Python: Improve lv property test coverage
In reply to Tony AslesonImprove the python unit test case to cover all of the properties of a LV and the properties of a LV segment.
In addition we also add a 'tag' to the lv so that we can retrieve it using the 'lv_tags' property to ensure that this works as expected.
--- test/api/python_lvm_unit.py | 56 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 53 insertions(+), 3 deletions(-)
test/api/python_lvm_unit.py +53 -3
@@ -21,7 +21,7 @@ import itertools import sys if sys.version_info[0] > 2: - long = int + long = int # Set of basic unit tests for the python bindings. #
@@ -309,7 +309,11 @@ class TestLvm(unittest.TestCase): def _test_prop(self, prop_obj, prop, var_type, settable): result = prop_obj.getProperty(prop) - self.assertEqual(type(result[0]), var_type) + #If we have no string value we can get a None type back + if result[0] is not None: + self.assertEqual(type(result[0]), var_type) + else: + self.assertTrue(str == var_type) self.assertEqual(type(result[1]), bool) self.assertTrue(result[1] == settable)
@@ -332,7 +336,53 @@ class TestLvm(unittest.TestCase): lv_name = 'lv_test' TestLvm._create_thin_lv(TestLvm._get_pv_device_names(), lv_name) lv, vg = TestLvm._get_lv(None, lv_name) - self._test_prop(lv, 'seg_count', long, False) + + lv_seg_properties = [ + ('chunk_size', long, False), ('devices', str, False), + ('discards', str, False), ('region_size', long, False), + ('segtype', str, False), ('seg_pe_ranges', str, False), + ('seg_size', long, False), ('seg_size_pe', long, False), + ('seg_start', long, False), ('seg_start_pe', long, False), + ('seg_tags', str, False), ('stripes', long, False), + ('stripe_size', long, False), ('thin_count', long, False), + ('transaction_id', long, False), ('zero', long, False)] + + lv_properties = [ + ('convert_lv', str, False), ('copy_percent', long, False), + ('data_lv', str, False), ('lv_attr', str, False), + ('lv_host', str, False), ('lv_kernel_major', long, False), + ('lv_kernel_minor', long, False), + ('lv_kernel_read_ahead', long, False), + ('lv_major', long, False), ('lv_minor', long, False), + ('lv_name', str, False), ('lv_path', str, False), + ('lv_profile', str, False), ('lv_read_ahead', long, False), + ('lv_size', long, False), ('lv_tags', str, False), + ('lv_time', str, False), ('lv_uuid', str, False), + ('metadata_lv', str, False), ('mirror_log', str, False), + ('lv_modules', str, False), ('move_pv', str, False), + ('origin', str, False), ('origin_size', long, False), + ('pool_lv', str, False), ('raid_max_recovery_rate', long, False), + ('raid_min_recovery_rate', long, False), + ('raid_mismatch_count', long, False), + ('raid_sync_action', str, False), + ('raid_write_behind', long, False), ('seg_count', long, False), + ('snap_percent', long, False), ('sync_percent', long, False)] + + # Generic test case, make sure we get what we expect + for t in lv_properties: + self._test_prop(lv, *t) + + segments = lv.listLVsegs() + if segments and len(segments): + for s in segments: + for t in lv_seg_properties: + self._test_prop(s, *t) + + # Test specific cases + tag = 'hello_world' + lv.addTag(tag) + tags = lv.getProperty('lv_tags') + self.assertTrue(tag in tags[0]) vg.close() def test_lv_tags(self):
Signature · 1 line
1.8.2.1