[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



Even in the case where it works with command line the pv->vg pointer is
pointing to the wrong vg.  The first 2 pv in the list point to the same
second vg.

The only reason the code is working today is that the display code
doesn't use the pv->vg pointer.  It uses the correct bits from the pv
pointer and retrieves the vg structure again.

What's the point of having a vg pointer in the pv structure if it isn't
correct?

-Tony


On 03/19/2013 10:52 PM, Tony Asleson wrote:
> 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 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 },


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]