[lvm-devel] [PATCH] Need another set of eyes to understand this

[Tony Asleson]

I'm working on a lvm2app library function to return a list of physical
volumes.  When I call get_pvs and use dm_list_iterate_items to walk the
returned list I'm finding that the pv->vg->vgmem is NULL for one of my
three physical volumes (First one) which causes problems as current code
assumes that this is valid and blindly dereferences it.

This is the same code path we use for the pvs command line and I'm at a
loss why I am getting this, unless I am missing something in the
cmd_context *cmd structure parameter.

Running in the debugger I can see that when walking the list of pv to add, the
pv and vg are correct, but when we do the _copy_pv we just copy the address
of the vg pointer.  We then do a release_vg and when we iterate to the next
pv to add we call vg_read_internal and assign the results to the same
vg local variable we end up stomping on the pv->vg->vgmem that is in the
results that we are going to return.

Can someone shed some light on how this was intended to work?

The command pvs returns this:

PV         VG             Fmt  Attr PSize  PFree
/dev/sdc                  lvm2 a--  18.00g 18.00g
/dev/sdd   vg-targetd-too lvm2 a--  18.00g 18.00g
/dev/sde   vg-targetd     lvm2 a--  18.00g 17.90g

vg-targetd is the first one to get added to the results list
and is the one that pvl->pv->vg-vgmem that is zero'd out, the
others are fine.

Thanks,
Tony

Signed-off-by: Tony Asleson <tasleson at redhat.com>
---
 liblvm/lvm2app.h | 15 +++++++++++++++
 liblvm/lvm_pv.c  | 17 +++++++++++++++++
 python/liblvm.c  | 54 ++++++++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 80 insertions(+), 6 deletions(-)

diff --git a/liblvm/lvm2app.h b/liblvm/lvm2app.h
index 98585c0..b07eff5 100644
--- a/liblvm/lvm2app.h
+++ b/liblvm/lvm2app.h
@@ -534,6 +534,21 @@ vg_t lvm_vg_create(lvm_t libh, const char *vg_name);
 struct dm_list *lvm_vg_list_lvs(vg_t vg);
 
 /**
+ * Return a list of PV handles for all.
+ *
+ * \memberof lvm_t
+ *
+ * \param   libh
+ * Library handle retrieved from lvm_init
+ *
+ * \return
+ * A list of lvm_pv_list structures containing pv handles for all physical
+ * volumes. If no PVs exist or a global lock was unable to be obtained a
+ * NULL is returned.
+ */
+struct dm_list *lvm_list_pvs(lvm_t libh);
+
+/**
  * Return a list of PV handles for a given VG handle.
  *
  * \memberof vg_t
diff --git a/liblvm/lvm_pv.c b/liblvm/lvm_pv.c
index bf9f630..3a8b148 100644
--- a/liblvm/lvm_pv.c
+++ b/liblvm/lvm_pv.c
@@ -17,6 +17,7 @@
 #include "lvm-string.h"
 #include "lvm_misc.h"
 #include "lvm2app.h"
+#include "locking.h"
 
 const char *lvm_pv_get_uuid(const pv_t pv)
 {
@@ -60,6 +61,22 @@ struct lvm_property_value lvm_pvseg_get_property(const pvseg_t pvseg,
 	return get_property(NULL, NULL, NULL, NULL, pvseg, name);
 }
 
+struct dm_list *lvm_list_pvs(lvm_t libh)
+{
+	struct dm_list *pvslist = NULL;
+	struct lvm_pv_list *pvl;
+	struct cmd_context *cmd = (struct cmd_context *)libh;
+
+	if (!lock_vol(cmd, VG_GLOBAL, LCK_VG_WRITE)) {
+		log_errno(ENOLCK, "Unable to obtain global lock.");
+	} else {
+		pvslist = get_pvs(cmd);
+		unlock_vg(cmd, VG_GLOBAL);
+	}
+
+	return pvslist;
+}
+
 struct dm_list *lvm_pv_list_pvsegs(pv_t pv)
 {
 	struct dm_list *list;
diff --git a/python/liblvm.c b/python/liblvm.c
index 906825e..d77fc9a 100644
--- a/python/liblvm.c
+++ b/python/liblvm.c
@@ -153,6 +153,45 @@ liblvm_lvm_list_vg_uuids(void)
 }
 
 static PyObject *
+liblvm_lvm_list_pvs(void)
+{
+	struct dm_list *pvs;
+	struct lvm_pv_list *pvl;
+	PyObject * pytuple;
+	pvobject * pvobj;
+	int i = 0;
+
+	LVM_VALID();
+
+	/* unlike other LVM api calls, if there are no results, we get NULL */
+	pvs = lvm_list_pvs(libh);
+	if (!pvs)
+		return Py_BuildValue("()");
+
+	pytuple = PyTuple_New(dm_list_size(pvs));
+	if (!pytuple)
+		return NULL;
+
+	dm_list_iterate_items(pvl, pvs) {
+		/* Create and initialize the object */
+		pvobj = PyObject_New(pvobject, &LibLVMpvType);
+		if (!pvobj) {
+			Py_DECREF(pytuple);
+			return NULL;
+		}
+
+		/* We don't have a parent vg object to be concerned about */
+		pvobj->parent_vgobj = NULL;
+
+		pvobj->pv = pvl->pv;
+		PyTuple_SET_ITEM(pytuple, i, (PyObject *) pvobj);
+		i++;
+	}
+
+	return pytuple;
+}
+
+static PyObject *
 liblvm_lvm_percent_to_float(PyObject *self, PyObject *arg)
 {
 	double converted;
@@ -1343,13 +1382,15 @@ liblvm_lvm_lv_snapshot(lvobject *self, PyObject *args)
 
 /* PV Methods */
 
-#define PV_VALID(pvobject)						\
-	do {								\
-		VG_VALID(pvobject->parent_vgobj);			\
-		if (!pvobject->pv) {					\
+#define PV_VALID(pvobject)								\
+	do {												\
+		if (pvobject->parent_vgobj) {					\
+			VG_VALID(pvobject->parent_vgobj);			\
+		}												\
+		if (!pvobject->pv) {							\
 			PyErr_SetString(PyExc_UnboundLocalError, "PV object invalid"); \
-			return NULL;					\
-		}							\
+			return NULL;								\
+		}												\
 	} while (0)
 
 static PyObject *
@@ -1550,6 +1591,7 @@ static PyMethodDef Liblvm_methods[] = {
 	{ "scan",		(PyCFunction)liblvm_lvm_scan, METH_NOARGS },
 	{ "listVgNames",	(PyCFunction)liblvm_lvm_list_vg_names, METH_NOARGS },
 	{ "listVgUuids",	(PyCFunction)liblvm_lvm_list_vg_uuids, METH_NOARGS },
+	{ "listPvs",		(PyCFunction)liblvm_lvm_list_pvs, METH_NOARGS },
 	{ "percentToFloat",	(PyCFunction)liblvm_lvm_percent_to_float, METH_VARARGS },
 	{ "vgNameFromPvid",	(PyCFunction)liblvm_lvm_vgname_from_pvid, METH_VARARGS },
 	{ "vgNameFromDevice",	(PyCFunction)liblvm_lvm_vgname_from_device, METH_VARARGS },
-- 
1.8.1.2