[PATCH 0/13] liblvm initialization, attribute, and object handling

[PATCH 0/13] liblvm initialization, attribute, and object handling

[Dave Wysochanski]

This is the latest in a series of liblvm patches involving the init code cleanup
as well as attribute and object handling code.  Patches are being developed
and reviewed by Thomas Woerner and myself.

I've begun to incorporate the latest vg_read() patches from Petr.
Note the "vg_open()" / "vg_close()" patches, which is a possible way we might
simplify some of those flags on vg_read() (an open/read/close sequence).
Patches are incomplete though I thought I'd throw it out for discussion.

[PATCH 01/13] Add system_dir to create_toolcontext().

[Dave Wysochanski]

Signed-off-by: Thomas Woerner <twoerner@redhat.com>
Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 daemons/clvmd/lvm-functions.c |    2 +-
 lib/commands/toolcontext.c    |    6 +++++-
 lib/commands/toolcontext.h    |    2 +-
 tools/lvmcmdline.c            |    2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/daemons/clvmd/lvm-functions.c b/daemons/clvmd/lvm-functions.c
index 5cf7eff..3db2756 100644
--- a/daemons/clvmd/lvm-functions.c
+++ b/daemons/clvmd/lvm-functions.c
@@ -724,7 +724,7 @@ void lvm_do_backup(const char *vgname)
 /* Called to initialise the LVM context of the daemon */
 int init_lvm(int using_gulm)
 {
-	if (!(cmd = create_toolcontext(1))) {
+	if (!(cmd = create_toolcontext(1, NULL))) {
 		log_error("Failed to allocate command context");
 		return 0;
 	}
diff --git a/lib/commands/toolcontext.c b/lib/commands/toolcontext.c
index 0a98325..c481a2b 100644
--- a/lib/commands/toolcontext.c
+++ b/lib/commands/toolcontext.c
@@ -998,7 +998,8 @@ static void _init_globals(struct cmd_context *cmd)
 }
 
 /* Entry point */
-struct cmd_context *create_toolcontext(unsigned is_long_lived)
+struct cmd_context *create_toolcontext(unsigned is_long_lived,
+				       const char *system_dir)
 {
 	struct cmd_context *cmd;
 
@@ -1030,6 +1031,9 @@ struct cmd_context *create_toolcontext(unsigned is_long_lived)
 
 	strcpy(cmd->sys_dir, DEFAULT_SYS_DIR);
 
+        if (system_dir)
+                strncpy(cmd->sys_dir, system_dir, PATH_MAX);
+
 	if (!_get_env_vars(cmd))
 		goto error;
 
diff --git a/lib/commands/toolcontext.h b/lib/commands/toolcontext.h
index 7d2aef9..ba6f1ed 100644
--- a/lib/commands/toolcontext.h
+++ b/lib/commands/toolcontext.h
@@ -94,7 +94,7 @@ struct cmd_context {
 	char sysfs_dir[PATH_MAX];
 };
 
-struct cmd_context *create_toolcontext(unsigned is_long_lived);
+struct cmd_context *create_toolcontext(unsigned is_long_lived, const char *system_dir);
 void destroy_toolcontext(struct cmd_context *cmd);
 int refresh_toolcontext(struct cmd_context *cmd);
 int config_files_changed(struct cmd_context *cmd);
diff --git a/tools/lvmcmdline.c b/tools/lvmcmdline.c
index 0b12750..2dabe64 100644
--- a/tools/lvmcmdline.c
+++ b/tools/lvmcmdline.c
@@ -1159,7 +1159,7 @@ struct cmd_context *init_lvm(void)
 
 	_cmdline.the_args = &_the_args[0];
 
-	if (!(cmd = create_toolcontext(0)))
+	if (!(cmd = create_toolcontext(0, NULL)))
 		return_NULL;
 
 	return cmd;
-- 
1.5.5.1

[PATCH 02/13] Add lvm_create, lvm_destroy, lvm_reload_config() APIs.

[Dave Wysochanski]

Add lvm2.c file.
Update test code.

Signed-off-by: Thomas Woerner <twoerner@redhat.com>
Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/Makefile.in |    3 +-
 lib/lvm2.c      |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/lvm2.h      |   44 +++++++++++++++++++++-------------
 test/api/test.c |   13 ++++-----
 4 files changed, 105 insertions(+), 25 deletions(-)
 create mode 100644 lib/lvm2.c

diff --git a/lib/Makefile.in b/lib/Makefile.in
index 54092cd..c0b58b9 100644
--- a/lib/Makefile.in
+++ b/lib/Makefile.in
@@ -86,7 +86,8 @@ SOURCES =\
 	report/report.c \
 	striped/striped.c \
 	uuid/uuid.c \
-	zero/zero.c
+	zero/zero.c \
+	lvm2.c
 
 ifeq ("@LVM1@", "internal")
   SOURCES +=\
diff --git a/lib/lvm2.c b/lib/lvm2.c
new file mode 100644
index 0000000..bb8f295
--- /dev/null
+++ b/lib/lvm2.c
@@ -0,0 +1,70 @@
+/*
+ * Copyright (C) 2004-2008 Red Hat, Inc. All rights reserved.
+ *
+ * This file is part of LVM2.
+ *
+ * This copyrighted material is made available to anyone wishing to use,
+ * modify, copy, or redistribute it subject to the terms and conditions
+ * of the GNU Lesser General Public License v.2.1.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include "lvm2.h"
+#include "lib.h"
+#include "toolcontext.h"
+#include "locking.h"
+#include "metadata-exported.h"
+#include "report.h"
+
+
+lvm_handle_t lvm_create(const char *system_dir)
+{
+	struct cmd_context *cmd;
+
+	/* To be done:
+	 * logging bound to handle
+	 */
+
+	/* create context */
+	cmd = create_toolcontext(1, system_dir);
+
+	/* initilize remaining */
+	if (cmd) {
+		int locking_type;
+
+		/* initilization from lvm_run_command */
+		init_error_message_produced(0);
+		sigint_clear();
+
+		/* get locking type from config tree */
+		/* FIXME: config option needed? */
+		locking_type = find_config_tree_int(cmd, "global/locking_type",
+						    1);
+
+		/* initialize locking */
+		if (! init_locking(locking_type, cmd)) {
+			printf("Locking type %d initialisation failed.",
+			       locking_type);
+			lvm_destroy((lvm_handle_t) cmd);
+			return NULL;
+		}
+	}
+
+	return (lvm_handle_t) cmd;
+}
+
+
+void lvm_destroy(lvm_handle_t libh)
+{
+	destroy_toolcontext((struct cmd_context *)libh);
+	/* no error handling here */
+}
+
+
+int lvm_reload_config(lvm_handle_t libh)
+{
+	return refresh_toolcontext((struct cmd_context *)libh);
+}
diff --git a/lib/lvm2.h b/lib/lvm2.h
index b18cead..f348035 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -18,36 +18,46 @@
 #include <stdint.h>
 
 /*
- * Library Initialisation
- * FIXME: For now just #define lvm2_create() and lvm2_destroy() to 
- * create_toolcontext() and destroy_toolcontext()
+ * lvm_handle_t
  */
-struct arg;
-struct cmd_context;
-struct cmd_context *create_toolcontext(unsigned is_long_lived);
-void destroy_toolcontext(struct cmd_context *cmd);
+struct lib_context; /* internal context data */
+typedef struct lib_context *lvm_handle_t;
 
+#ifdef FUTURE_FEATURE
 /*
- * lvm2_create
-lvm_handle_t lvm2_create(void);
+ * lvm_config_t
+ */
+struct lvm_config {
+	const char *system_dir;
+	unsigned is_long_lived;
+};
+#endif /* FUTURE_FEATURE */
+
+/*
+ * lvm_create
  *
- * Description: Create an LVM2 handle used in many other APIs.
+ * Description: Create an LVM handle used in many other APIs.
  *
  * Returns:
  * NULL: Fail - unable to initialise handle.
- * non-NULL: Success - valid LVM2 handle returned
+ * non-NULL: Success - valid LVM handle returned
  */
-#define lvm2_create(X) create_toolcontext(1)
+lvm_handle_t lvm_create(const char *system_dir);
 
 /*
- * lvm2_destroy
-void lvm2_destroy(lvm_handle_t h);
+ * lvm_destroy
  *
- * Description: Destroy an LVM2 handle allocated with lvm2_create
+ * Description: Destroy an LVM handle allocated with lvm_create
  *
  * Parameters:
- * - h (IN): handle obtained from lvm2_create
+ * - h (IN): handle obtained from lvm_create
+ */
+void lvm_destroy(lvm_handle_t libh);
+
+/*
+ * lvm_reload_config
+ * Reload configuration files
  */
-#define lvm2_destroy(X) destroy_toolcontext(X)
+int lvm_reload_config(lvm_handle_t libh);
 
 #endif
diff --git a/test/api/test.c b/test/api/test.c
index de53c46..9411663 100644
--- a/test/api/test.c
+++ b/test/api/test.c
@@ -48,7 +48,7 @@ static int lvm_split(char *str, int *argc, char **argv, int max)
 	return *argc;
 }
 
-static int lvmapi_test_shell(void *h)
+static int lvmapi_test_shell(lvm_handle_t libh)
 {
 	int argc, i;
 	char *input = NULL, *args[MAX_ARGS], **argv;
@@ -99,18 +99,17 @@ static int lvmapi_test_shell(void *h)
 		      
 int main (int argc, char *argv[])
 {
-	void *h;
+	lvm_handle_t libh;
 
-	h = lvm2_create();
-	if (!h) {
+	libh = lvm_create(NULL);
+	if (!libh) {
 		printf("Unable to open lvm library instance\n");
 		return 1;
 	}
 
-	lvmapi_test_shell(h);
+	lvmapi_test_shell(libh);
 
-	if (h)
-		lvm2_destroy(h);
+	lvm_destroy(libh);
 	return 0;
 }
 
-- 
1.5.5.1

[PATCH 03/13] Move vg_t, lv_t, and pv_t from metadata-exported.h into lvm2.h

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/lvm2.h                       |    9 +++++++++
 lib/metadata/metadata-exported.h |    8 +-------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/lib/lvm2.h b/lib/lvm2.h
index f348035..bfa1852 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -60,4 +60,13 @@ void lvm_destroy(lvm_handle_t libh);
  */
 int lvm_reload_config(lvm_handle_t libh);
 
+struct volume_group;
+typedef struct volume_group vg_t;
+
+struct physical_volume;
+typedef struct physical_volume pv_t;
+
+struct logical_volume;
+typedef struct logical_volume lv_t;
+
 #endif
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index a1c45ce..257bf91 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -22,13 +22,7 @@
 #define _LVM_METADATA_EXPORTED_H
 
 #include "uuid.h"
-
-struct physical_volume;
-typedef struct physical_volume pv_t;
-struct volume_group;
-typedef struct volume_group vg_t;
-
-struct logical_volume;
+#include "lvm2.h"
 
 struct lv_segment;
 struct pv_segment;
-- 
1.5.5.1

[PATCH 04/13] Add lvm_pv_name, lvm_vg_name, and lvm_lv_name accessors.

[Dave Wysochanski]

RFC - liblvm could provide accessor functions such as these along with
the generic attribute functions.  These specific accessor functions will
make application code simpler and more readable and may be desired by
application programmers over the generic attribute functions for single
attributes.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/lvm2.h              |    7 +++++++
 lib/metadata/metadata.c |   15 +++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/lib/lvm2.h b/lib/lvm2.h
index bfa1852..5f44614 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -69,4 +69,11 @@ typedef struct physical_volume pv_t;
 struct logical_volume;
 typedef struct logical_volume lv_t;
 
+/*
+ * PV, VG, and LV object attribute functions.
+ */
+const char *lvm_pv_name(const pv_t *pv);
+const char *lvm_vg_name(const vg_t *vg);
+const char *lvm_lv_name(const lv_t *lv);
+
 #endif
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index a2b5f2f..4c9f939 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2673,6 +2673,21 @@ vg_t *vg_read_for_update(struct cmd_context *cmd, const char *vg_name,
 	return vg_read(cmd, vg_name, vgid, flags | READ_FOR_UPDATE);
 }
 
+const char *lvm_pv_name(const pv_t *pv)
+{
+	return dev_name(pv->dev);
+}
+
+const char *lvm_vg_name(const vg_t *vg)
+{
+	return vg->name;
+}
+
+const char *lvm_lv_name(const lv_t *lv)
+{
+	return lv->name;
+}
+
 /*
  * Test the validity of a VG handle returned by vg_read() or vg_read_for_update().
  *
-- 
1.5.5.1

[PATCH 05/13] Add lvm_vg_open().

[Dave Wysochanski]

RFC - lvm_vg_open().  The new vg_read() function is a step in the right
direction but still contains a lot of new options / flags that will
be new to people and we can hopefully simplify.  lvm_vg_open() was
modelled after 'open' which gives us a potential model that others are
familiar with.  Can we use the modes and flags of 'open' in a similar way
or do we need to introduce new flags/concepts to people?  For example,
can we use a some of the open flags such as O_CREAT and O_EXCL to
implement READ_CHECK_EXISTENCE?

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/lvm2.h              |    2 ++
 lib/metadata/metadata.c |   14 ++++++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/lib/lvm2.h b/lib/lvm2.h
index 5f44614..20c8ce0 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -16,6 +16,7 @@
 #define _LIB_LVM2_H
 
 #include <stdint.h>
+#include <fcntl.h>
 
 /*
  * lvm_handle_t
@@ -76,4 +77,5 @@ const char *lvm_pv_name(const pv_t *pv);
 const char *lvm_vg_name(const vg_t *vg);
 const char *lvm_lv_name(const lv_t *lv);
 
+vg_t *lvm_vg_open(lvm_handle_t libh, const char *vg_name, mode_t mode);
 #endif
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 4c9f939..38b6e09 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2472,6 +2472,20 @@ vg_t *vg_lock_and_read(struct cmd_context *cmd, const char *vg_name,
 }
 
 /*
+ * Open / read a VG.
+ * FIXME: Only read access allowed.
+ */
+vg_t *lvm_vg_open(lvm_handle_t libh, const char *vg_name, mode_t mode)
+{
+	if ((mode & O_ACCMODE) != O_RDONLY) {
+		log_error("Invalid access mode 0x%x for lvm_vg_read()\n",
+			mode);
+		return NULL;
+	}
+	return vg_read((struct cmd_context *)libh, vg_name, NULL, 0);
+}
+
+/*
  * Create a (vg_t) volume group handle from a struct volume_group pointer and a
  * possible failure code or zero for success.
  */
-- 
1.5.5.1

[PATCH 06/13] Add lvm_vg_close().

[Dave Wysochanski]

For now we cannot free the handle but just check to see if we should
unlock it.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/lvm2.h              |    1 +
 lib/metadata/metadata.c |   11 +++++++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/lib/lvm2.h b/lib/lvm2.h
index 20c8ce0..85014f5 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -78,4 +78,5 @@ const char *lvm_vg_name(const vg_t *vg);
 const char *lvm_lv_name(const lv_t *lv);
 
 vg_t *lvm_vg_open(lvm_handle_t libh, const char *vg_name, mode_t mode);
+void lvm_vg_close(vg_t *vg);
 #endif
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 38b6e09..0fa5db4 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2486,6 +2486,17 @@ vg_t *lvm_vg_open(lvm_handle_t libh, const char *vg_name, mode_t mode)
 }
 
 /*
+ * Close a VG handle.
+ * If lock is still held, unlock the VG.
+ * FIXME: We cannot free the memory as it's tied to the cmd->mem pool.
+ */
+void lvm_vg_close(vg_t *vg)
+{
+	if (vgname_is_locked(vg->name))
+		unlock_vg(vg->cmd, vg->name);
+}
+
+/*
  * Create a (vg_t) volume group handle from a struct volume_group pointer and a
  * possible failure code or zero for success.
  */
-- 
1.5.5.1

[PATCH 07/13] Add lvm_vg_get_attr_list() libLVM API to return a list of VG attribute names.

[Dave Wysochanski]

This API returns a list of attribute names for a VG.  A later API will
allow the values of the attributes to be queried.  The API implementation
relies on adding an exported call to libdevmapper, dm_report_get_field_ids(),
which is the interface into dm's reporting infrastructure to retrieve the
attribute/field names/ids.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/lvm2.h              |    3 +++
 lib/report/report.c     |   28 ++++++++++++++++++++++++++++
 libdm/.exported_symbols |    1 +
 libdm/libdevmapper.h    |   12 ++++++++++++
 libdm/libdm-report.c    |   27 +++++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/lib/lvm2.h b/lib/lvm2.h
index 85014f5..57e8b6c 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -17,6 +17,7 @@
 
 #include <stdint.h>
 #include <fcntl.h>
+#include "libdevmapper.h"
 
 /*
  * lvm_handle_t
@@ -77,6 +78,8 @@ const char *lvm_pv_name(const pv_t *pv);
 const char *lvm_vg_name(const vg_t *vg);
 const char *lvm_lv_name(const lv_t *lv);
 
+int lvm_vg_get_attr_list(vg_t *vg, struct dm_list *list);
+
 vg_t *lvm_vg_open(lvm_handle_t libh, const char *vg_name, mode_t mode);
 void lvm_vg_close(vg_t *vg);
 #endif
diff --git a/lib/report/report.c b/lib/report/report.c
index 8439b2b..8e733d8 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -1191,3 +1191,31 @@ int report_object(void *handle, struct volume_group *vg,
 
 	return dm_report_object(handle, &obj);
 }
+
+/*
+ * Get a list of attributes for a vg
+ */
+int lvm_vg_get_attr_list(vg_t *vg, struct dm_list *list)
+{
+	void *rh;
+	report_type_t report_type = VGS;
+
+	dm_list_init(list);
+
+	/*
+	 * Create a report so we can return the headings.
+	 */
+	if (!(rh = report_init(vg->cmd, "all", "", &report_type,
+			       " ", 1, 1, 1, 0, 0, 0))) {
+		stack;
+		return 0;
+	}
+
+	if (!dm_report_get_field_ids(rh, report_type, list)) {
+		dm_report_free(rh);
+		return 0;
+	}
+
+	dm_report_free(rh);
+	return 1;
+}
diff --git a/libdm/.exported_symbols b/libdm/.exported_symbols
index a6e04f8..aa70f29 100644
--- a/libdm/.exported_symbols
+++ b/libdm/.exported_symbols
@@ -132,6 +132,7 @@ dm_report_field_int32
 dm_report_field_uint32
 dm_report_field_uint64
 dm_report_field_set_value
+dm_report_get_field_ids
 dm_report_set_output_field_name_prefix
 dm_regex_create
 dm_regex_match
diff --git a/libdm/libdevmapper.h b/libdm/libdevmapper.h
index 93aecbb..cc8302a 100644
--- a/libdm/libdevmapper.h
+++ b/libdm/libdevmapper.h
@@ -924,6 +924,17 @@ struct dm_report_field_type {
 	const char *desc;	/* description of the field */
 };
 
+struct dm_report_field_ids_type {
+	struct dm_list list;
+	const char *id;
+};
+
+/*
+ * Returns a list of all field ids for a specific report type.
+ */
+int dm_report_get_field_ids(struct dm_report *rh, uint32_t type,
+			    struct dm_list *list);
+
 /*
  * dm_report_init output_flags
  */
@@ -947,6 +958,7 @@ int dm_report_object(struct dm_report *rh, void *object);
 int dm_report_output(struct dm_report *rh);
 void dm_report_free(struct dm_report *rh);
 
+
 /*
  * Prefix added to each field name with DM_REPORT_OUTPUT_FIELD_NAME_PREFIX
  */
diff --git a/libdm/libdm-report.c b/libdm/libdm-report.c
index 0c5ee6d..a9db211 100644
--- a/libdm/libdm-report.c
+++ b/libdm/libdm-report.c
@@ -1076,3 +1076,30 @@ int dm_report_output(struct dm_report *rh)
 	else
 		return _output_as_columns(rh);
 }
+
+/*
+ * Return a list of field IDs (names) for a specified report type.
+ */
+int dm_report_get_field_ids(struct dm_report *rh, uint32_t type,
+			    struct dm_list *list)
+{
+	uint32_t f;
+	struct dm_report_field_ids_type *field_id;
+
+	for (f = 0; rh->fields[f].report_fn; f++) {
+		if (rh->fields[f].type != type)
+			continue;
+
+		if (!(field_id = dm_pool_zalloc(rh->mem, sizeof(*field_id)))) {
+			log_error("dm_report_get_field_ids: struct "
+				  "dm_report_field_ids_type allocation failed");
+			return 0;
+		}
+		if (!(field_id->id = dm_pool_strdup(rh->mem, rh->fields[f].id))) {
+			log_error("dm_report_get_field_ids: dm_pool_strdup failed");
+			return 0;
+		}
+		dm_list_add(list, &field_id->list);
+	}
+	return 1;
+}
-- 
1.5.5.1

[PATCH 08/13] Add lvm_vg_get_attr_value() libLVM API to query to value of a VG attribute.

[Dave Wysochanski]

This API may be used independently to query the value of a VG attribute,
provided the name of the attribute is known.  If the name of the attribute(s)
are unknown, the previous API, vg_get_attr_list(), may be used.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/lvm2.h              |    2 ++
 lib/report/report.c     |   28 ++++++++++++++++++++++++++++
 libdm/.exported_symbols |    1 +
 libdm/libdevmapper.h    |   12 ++++++++++++
 libdm/libdm-report.c    |   29 +++++++++++++++++++++++++++++
 5 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/lib/lvm2.h b/lib/lvm2.h
index 57e8b6c..b274762 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -79,6 +79,8 @@ const char *lvm_vg_name(const vg_t *vg);
 const char *lvm_lv_name(const lv_t *lv);
 
 int lvm_vg_get_attr_list(vg_t *vg, struct dm_list *list);
+int lvm_vg_get_attr_value(vg_t *vg, const char *attr_name,
+			  struct dm_report_field_value_type *value);
 
 vg_t *lvm_vg_open(lvm_handle_t libh, const char *vg_name, mode_t mode);
 void lvm_vg_close(vg_t *vg);
diff --git a/lib/report/report.c b/lib/report/report.c
index 8e733d8..1216fff 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -1219,3 +1219,31 @@ int lvm_vg_get_attr_list(vg_t *vg, struct dm_list *list)
 	dm_report_free(rh);
 	return 1;
 }
+
+/*
+ * Get the value of the VG attribute 'attr_name'.
+ */
+int lvm_vg_get_attr_value(vg_t *vg, const char *attr_name,
+			  struct dm_report_field_value_type *value)
+{
+	void *rh;
+	report_type_t report_type = VGS;
+
+	if (!(rh = report_init(vg->cmd, attr_name, "", &report_type,
+			       " ", 1, 1, 1, 0, 0, 0))) {
+		stack;
+		return 0;
+	}
+
+	if (!report_object(rh, vg, NULL, NULL, NULL, NULL)) {
+		stack;
+		return 0;
+	}
+	
+	if (!dm_report_get_field_value(rh, value)) {
+		stack;
+		return 0;
+	}
+	dm_report_free(rh);
+	return 1;
+}
diff --git a/libdm/.exported_symbols b/libdm/.exported_symbols
index aa70f29..e7a4906 100644
--- a/libdm/.exported_symbols
+++ b/libdm/.exported_symbols
@@ -133,6 +133,7 @@ dm_report_field_uint32
 dm_report_field_uint64
 dm_report_field_set_value
 dm_report_get_field_ids
+dm_report_get_field_value
 dm_report_set_output_field_name_prefix
 dm_regex_create
 dm_regex_match
diff --git a/libdm/libdevmapper.h b/libdm/libdevmapper.h
index cc8302a..d6cb7cf 100644
--- a/libdm/libdevmapper.h
+++ b/libdm/libdevmapper.h
@@ -927,6 +927,15 @@ struct dm_report_field_type {
 struct dm_report_field_ids_type {
 	struct dm_list list;
 	const char *id;
+	uint32_t type; /* FIXME: unnecessary ?*/
+};
+
+struct dm_report_field_value_type {
+	unsigned is_string;
+	union {
+		char *s_val;
+		uint64_t n_val;
+	} u;
 };
 
 /*
@@ -935,6 +944,9 @@ struct dm_report_field_ids_type {
 int dm_report_get_field_ids(struct dm_report *rh, uint32_t type,
 			    struct dm_list *list);
 
+int dm_report_get_field_value(struct dm_report *rh,
+			      struct dm_report_field_value_type *value);
+
 /*
  * dm_report_init output_flags
  */
diff --git a/libdm/libdm-report.c b/libdm/libdm-report.c
index a9db211..625e212 100644
--- a/libdm/libdm-report.c
+++ b/libdm/libdm-report.c
@@ -1099,7 +1099,36 @@ int dm_report_get_field_ids(struct dm_report *rh, uint32_t type,
 			log_error("dm_report_get_field_ids: dm_pool_strdup failed");
 			return 0;
 		}
+		field_id->type = type;
 		dm_list_add(list, &field_id->list);
 	}
 	return 1;
 }
+
+
+int dm_report_get_field_value(struct dm_report *rh,
+			      struct dm_report_field_value_type *value)
+{
+	struct dm_report_field *field;
+	struct row *row;
+
+	dm_list_iterate_items(row, &rh->rows) {
+		if ((field = dm_list_item(dm_list_first(&row->fields),
+					  struct dm_report_field))) {
+			if ((field->props->flags & DM_REPORT_FIELD_TYPE_MASK) ==
+			    DM_REPORT_FIELD_TYPE_STRING) {
+				value->is_string = 1;
+				value->u.s_val = dm_pool_strdup(rh->mem,
+								field->sort_value);
+			} else {
+				value->is_string = 0;
+				value->u.n_val = *(const uint64_t *)field->sort_value;
+			}
+			dm_list_del(&field->list);
+		}
+		
+	}
+
+	return 1;
+
+}
-- 
1.5.5.1

[PATCH 09/13] Add lvm_lvs_in_vg() API.

[Dave Wysochanski]

Add exported lvm_lv_list structure along with the API.  Note that this
struct is very similar to struct lv_list.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/lvm2.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/lib/lvm2.h b/lib/lvm2.h
index b274762..4d25d25 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -84,4 +84,11 @@ int lvm_vg_get_attr_value(vg_t *vg, const char *attr_name,
 
 vg_t *lvm_vg_open(lvm_handle_t libh, const char *vg_name, mode_t mode);
 void lvm_vg_close(vg_t *vg);
+
+struct lvm_lv_list {
+	struct dm_list list;
+	lv_t *lv;
+};
+struct dm_list *lvm_lvs_in_vg(vg_t *vg);
+
 #endif
-- 
1.5.5.1

[PATCH 10/13] Add lvm_pvs_in_vg().

[Dave Wysochanski]

NOTE: struct lvm_pv_list should not be necessary but I did not like exposing
struct pv_list and attempts to clean up struct pv_list to contain only
struct dm_list and pv_t members were unsuccessful.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/lvm2.h              |   10 ++++++++++
 lib/metadata/metadata.c |   10 ++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/lib/lvm2.h b/lib/lvm2.h
index 4d25d25..25ce12d 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -91,4 +91,14 @@ struct lvm_lv_list {
 };
 struct dm_list *lvm_lvs_in_vg(vg_t *vg);
 
+/*
+ * FIXME: struct lvm_pv_list is not equivalent to struct pv_list, which
+ * is used in lvm_pvs_in_vg().  Ideally we should not need a new structure.
+ */
+struct lvm_pv_list {
+	struct dm_list list;
+	pv_t *pv;
+};
+struct dm_list *lvm_pvs_in_vg(vg_t *vg);
+
 #endif
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 0fa5db4..450e781 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2496,6 +2496,16 @@ void lvm_vg_close(vg_t *vg)
 		unlock_vg(vg->cmd, vg->name);
 }
 
+struct dm_list *lvm_lvs_in_vg(vg_t *vg)
+{
+	return (&vg->lvs);
+}
+
+struct dm_list *lvm_pvs_in_vg(vg_t *vg)
+{
+	return (&vg->pvs);
+}
+
 /*
  * Create a (vg_t) volume group handle from a struct volume_group pointer and a
  * possible failure code or zero for success.
-- 
1.5.5.1

[PATCH 11/13] Add lvm_lv_get_attr_list() and lvm_lv_get_attr_value().

[Dave Wysochanski]

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/lvm2.h          |    3 ++
 lib/report/report.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/lib/lvm2.h b/lib/lvm2.h
index 25ce12d..2a5dd9e 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -91,6 +91,9 @@ struct lvm_lv_list {
 };
 struct dm_list *lvm_lvs_in_vg(vg_t *vg);
 
+int lvm_lv_get_attr_list(lv_t *lv, struct dm_list *list);
+int lvm_lv_get_attr_value(lv_t *lv, const char *attr_name,
+			  struct dm_report_field_value_type *value);
 /*
  * FIXME: struct lvm_pv_list is not equivalent to struct pv_list, which
  * is used in lvm_pvs_in_vg().  Ideally we should not need a new structure.
diff --git a/lib/report/report.c b/lib/report/report.c
index 1216fff..70bc3bd 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -1247,3 +1247,60 @@ int lvm_vg_get_attr_value(vg_t *vg, const char *attr_name,
 	dm_report_free(rh);
 	return 1;
 }
+
+
+/*
+ * Get a list of attributes for a LV
+ */
+int lvm_lv_get_attr_list(lv_t *lv, struct dm_list *list)
+{
+	void *rh;
+	report_type_t report_type = LVS;
+
+	dm_list_init(list);
+
+	/*
+	 * Create a report so we can return the headings.
+	 */
+	if (!(rh = report_init(lv->vg->cmd, "all", "", &report_type,
+			       " ", 1, 1, 1, 0, 0, 0))) {
+		stack;
+		return 0;
+	}
+
+	if (!dm_report_get_field_ids(rh, report_type, list)) {
+		dm_report_free(rh);
+		return 0;
+	}
+
+	dm_report_free(rh);
+	return 1;
+}
+
+/*
+ * Get the value of the LV attribute 'attr_name'.
+ */
+int lvm_lv_get_attr_value(lv_t *lv, const char *attr_name,
+			  struct dm_report_field_value_type *value)
+{
+	void *rh;
+	report_type_t report_type = LVS;
+
+	if (!(rh = report_init(lv->vg->cmd, attr_name, "", &report_type,
+			       " ", 1, 1, 1, 0, 0, 0))) {
+		stack;
+		return 0;
+	}
+
+	if (!report_object(rh, NULL, lv, NULL, NULL, NULL)) {
+		stack;
+		return 0;
+	}
+	
+	if (!dm_report_get_field_value(rh, value)) {
+		stack;
+		return 0;
+	}
+	dm_report_free(rh);
+	return 1;
+}
-- 
1.5.5.1

[PATCH 12/13] First cut at adding pv_obj_* APIs.

[Dave Wysochanski]

Impelemnt pv_obj_get(), pv_obj_get_list(), pv_obj_get_attr_list() and
pv_obj_get_attr_value().

This is a first attempt at adding pv_obj_* APIs as outlined by
http://fedoraproject.org/wiki/LVM/liblvm/api

Why add another structure when we could use struct physical_volume
and other internal structs?  At this point it is not clear but would
provide us with the ability to more easily allocate and free handles,
and would allow places to store things like the access mode and API
errors without polluting the internal structures.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/lvm2.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/lvm2.h |   36 ++++++++++++++++++
 2 files changed, 157 insertions(+), 0 deletions(-)

diff --git a/lib/lvm2.c b/lib/lvm2.c
index bb8f295..4108321 100644
--- a/lib/lvm2.c
+++ b/lib/lvm2.c
@@ -68,3 +68,124 @@ int lvm_reload_config(lvm_handle_t libh)
 {
 	return refresh_toolcontext((struct cmd_context *)libh);
 }
+
+
+struct lvm_pv_obj {
+	char mode;
+	struct physical_volume *pv;
+};
+
+
+/*
+ * Gets object for a physical volume by volume path
+ * Retuns lvm_pv_obj_t on success, otherwide NULL. errno is used to indicate 
+ * the error.
+ * mode: "r" or "w"
+ */
+lvm_pv_obj_t *lvm_pv_obj_get(lvm_handle_t libh, const char* phys_vol,
+                            const char *mode)
+{
+	struct cmd_context *cmd = (struct cmd_context *)libh;
+	lvm_pv_obj_t *pv_obj;
+	struct physical_volume *pv;
+	struct dm_list mdas;
+	uint64_t label_sector;
+
+	dm_list_init(&mdas);
+	pv = pv_read(cmd, phys_vol, &mdas, &label_sector, 0);
+	if (!pv)
+		return NULL;
+	
+	pv_obj = dm_pool_alloc(cmd->mem, sizeof(*pv_obj));
+	if (!pv_obj)
+		return NULL;
+
+	pv_obj->pv = pv;
+	pv_obj->mode = mode[0];
+
+	return pv_obj;
+}
+
+
+lvm_pv_obj_list_t *lvm_pv_obj_list(lvm_handle_t libh)
+{
+	struct dm_list *pvs;
+	struct pv_list *pvl;
+	lvm_pv_obj_list_t *head = NULL, *elem;
+	struct cmd_context *cmd = (struct cmd_context *)libh;
+
+	pvs = get_pvs(cmd);
+
+	if (dm_list_empty(pvs))
+		return NULL;
+
+	dm_list_iterate_items(pvl, pvs) {
+		elem = dm_pool_alloc(cmd->mem, sizeof(*elem));
+		if (!elem)
+			return NULL;
+		elem->obj = lvm_pv_obj_get(libh, lvm_pv_name(pvl->pv), "r");
+		if (!head)
+			head = elem;
+		dm_list_add(&head->list, &elem->list);
+	}
+	return head;
+}
+
+
+/*
+ * Get a list of attributes for a vg
+ */
+int lvm_pv_obj_get_attr_list(lvm_pv_obj_t *obj, struct dm_list *list)
+{
+	void *rh;
+	report_type_t report_type = PVS;
+
+	dm_list_init(list);
+
+	/*
+	 * Create a report so we can return the headings.
+	 */
+	if (!(rh = report_init(obj->pv->fmt->cmd, "all", "", &report_type,
+			       " ", 1, 1, 1, 0, 0, 0))) {
+		stack;
+		return 0;
+	}
+
+	if (!dm_report_get_field_ids(rh, report_type, list)) {
+		dm_report_free(rh);
+		return 0;
+	}
+
+	dm_report_free(rh);
+	return 1;
+}
+
+/*
+ * Get the value of the VG attribute 'attr_name'.
+ */
+int lvm_pv_obj_get_attr_value(lvm_pv_obj_t *obj, const char *attr_name,
+			  struct dm_report_field_value_type *value)
+{
+	void *rh;
+	report_type_t report_type = PVS;
+
+	if (!(rh = report_init(obj->pv->fmt->cmd, attr_name, "", &report_type,
+			       " ", 1, 1, 1, 0, 0, 0))) {
+		stack;
+		return 0;
+	}
+
+	if (!report_object(rh, NULL, NULL, obj->pv, NULL, NULL)) {
+		stack;
+		return 0;
+	}
+	
+	if (!dm_report_get_field_value(rh, value)) {
+		stack;
+		return 0;
+	}
+	dm_report_free(rh);
+	return 1;
+}
+
+
diff --git a/lib/lvm2.h b/lib/lvm2.h
index 2a5dd9e..a47c0a5 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -104,4 +104,40 @@ struct lvm_pv_list {
 };
 struct dm_list *lvm_pvs_in_vg(vg_t *vg);
 
+/*
+ * The lvm physical volume object.
+ */
+struct lvm_pv_obj;
+typedef struct lvm_pv_obj lvm_pv_obj_t; /* with mode inside (r/rw) */
+
+/*
+ * The lvm physical volume object list.
+ */
+typedef struct lvm_pv_obj_list {
+  struct dm_list list;
+  lvm_pv_obj_t *obj;
+} lvm_pv_obj_list_t;
+
+/*
+ * Gets object for a physical volume by volume path
+ * Retuns lvm_pv_obj_t on success, otherwide NULL. errno is used to indicate 
+ * the error.
+ * mode: "r" or "rw"
+ */
+lvm_pv_obj_t *lvm_pv_obj_get(lvm_handle_t libh, const char* phys_vol,
+                            const char *mode);
+
+
+/*
+ * Returns list of physical volumes.
+ * Retuns lvm_pv_obj_list_t on success, otherwide NULL. errno is used to 
+ * indicate the error. The objects are read only.
+ */
+lvm_pv_obj_list_t *lvm_pv_obj_list(lvm_handle_t libh);
+
+int lvm_pv_obj_get_attr_list(lvm_pv_obj_t *obj, struct dm_list *list);
+int lvm_pv_obj_get_attr_value(lvm_pv_obj_t *obj, const char *attr_name,
+			      struct dm_report_field_value_type *value);
+
+
 #endif
-- 
1.5.5.1

[PATCH 13/13] Add test code.

[Dave Wysochanski]

Test lvm_pv_list, lvm_lv_get_attr_list, lvm_lv_get_attr_value().
pv_obj_get(), pv_obj_get_attr_list(), pv_obj_get_attr_value,
vg_get_attr_list and vg_get_attr_value,
lv_get_attr_list and lv_get_attr_value,
lvs_in_vg(), pvs_in_vg()
lvm_vg_open, lvm_vg_close().

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 test/api/test.c |  322 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 317 insertions(+), 5 deletions(-)

diff --git a/test/api/test.c b/test/api/test.c
index 9411663..b996770 100644
--- a/test/api/test.c
+++ b/test/api/test.c
@@ -48,10 +48,299 @@ static int lvm_split(char *str, int *argc, char **argv, int max)
 	return *argc;
 }
 
+static void _show_help(void)
+{
+	printf("'pv_obj_get pvname': Issue a lvm_pv_obj_get() API call on PV 'pvname'\n");
+	printf("'pv_obj_get_attr_list': Issue lvm_pv_obj_get_attr_list() API; must follow pv_obj_get\n");
+	printf("'pv_obj_get_attr_value [attr_name]': Issue lvm_pv_obj_get_attr_value() API; must follow pv_obj_get\n");
+	printf("'vg_open vgname': Issue a lvm_vg_open() API call on VG 'vgname'\n");
+	printf("'vg_close vgname': Issue a lvm_vg_close() API call on VG 'vgname'\n");
+	printf("'vg_get_attr_list': Issue lvm_vg_get_attr_list() API; must follow vg_open\n");
+	printf("'vg_get_attr_value [attr_name]': Issue lvm_vg_get_attr_value() API; must follow vg_open\n");
+	printf("'pvs_in_vg': Issue lvm_pvs_in_vg() API; must follow vg_open\n");
+	printf("'lv_get_attr_list': Issue lvm_lv_get_attr_list() API; must follow vg_open\n");
+	printf("'lv_get_attr_value [attr_name]': Issue lvm_lv_get_attr_value() API; must follow vg_open\n");
+	printf("'quit': exit the program\n");
+}
+
+static void _print_attr(const char *name,
+			struct dm_report_field_value_type value)
+{
+	if (value.is_string)
+		printf("%s = %s\n", name, value.u.s_val);
+	else
+		printf("%s = %llu\n", name, (unsigned long long)value.u.n_val);
+}
+
+static void get_and_print_vg_attr(vg_t *vg, const char *name)
+{
+	struct dm_report_field_value_type value;
+
+	if (!lvm_vg_get_attr_value(vg, name, &value)) {
+		printf("Error reading vg attribute %s value \n",
+		       name);
+		return;
+	}
+	_print_attr(name, value);
+}
+
+static void get_and_print_lv_attr(lv_t *lv, const char *name)
+{
+	struct dm_report_field_value_type value;
+
+	if (!lvm_lv_get_attr_value(lv, name, &value)) {
+		printf("Error reading lv attribute %s value \n",
+		       name);
+		return;
+	}
+	_print_attr(name, value);
+}
+
+static void get_and_print_pv_attr(lvm_pv_obj_t *pv, const char *name)
+{
+	struct dm_report_field_value_type value;
+
+	if (!lvm_pv_obj_get_attr_value(pv, name, &value)) {
+		printf("Error reading pv attribute %s value \n",
+		       name);
+		return;
+	}
+	_print_attr(name, value);
+}
+
+static void _vg_open(char **argv, int argc, vg_t **vgp, void *h)
+{
+	if (argc < 2) {
+		printf ("Please enter vg_name\n");
+		return;
+	}
+	*vgp = lvm_vg_open(h, argv[1], O_RDONLY);
+	if (vg_read_error(*vgp))
+		printf("Error opening vg %s\n", argv[1]);
+	else
+		printf("Success opening vg %s\n", argv[1]);
+}
+
+static void _pv_obj_get(char **argv, int argc, lvm_pv_obj_t **pvp, void *h)
+{
+	if (argc < 2) {
+		printf ("Please enter pv_name\n");
+		return;
+	}
+	if (!(*pvp = lvm_pv_obj_get(h, argv[1], "r")))
+		printf("Error opening PV %s\n", argv[1]);
+	else
+		printf("Success opening PV %s\n", argv[1]);
+}
+
+static void _pv_obj_get_attr_list(lvm_pv_obj_t *pv, void *h)
+{
+	struct dm_report_field_ids_type *field;
+	struct dm_list fields;
+
+	if (!pv) {
+		printf("Run pv_obj_get first\n");
+		return;
+			}
+	if (!lvm_pv_obj_get_attr_list(pv, &fields)) {
+		printf("Error reading pv attribute list\n");
+		return;
+	}
+	printf("PV attribute names:\n");
+	dm_list_iterate_items(field, &fields) {
+		printf("%s\n", field->id);
+	}
+}
+
+static void _pv_obj_get_attr_value(char **argv, int argc,
+				   lvm_pv_obj_t *pv)
+{
+	struct dm_report_field_ids_type *field;
+	struct dm_list fields;
+	int i;
+
+	if (!pv) {
+		printf("Run pv_obj_get first\n");
+		return;
+	}
+	if (argc > 1) {
+		for (i=1; i<argc; i++)
+			get_and_print_pv_attr(pv, argv[i]);
+		return;
+	}
+	if (!lvm_pv_obj_get_attr_list(pv, &fields)) {
+		printf("Error reading pv attribute list\n");
+		return;
+	}
+	dm_list_iterate_items(field, &fields) {
+		get_and_print_pv_attr(pv, field->id);
+	}
+}
+
+static void _vg_get_attr_list(vg_t *vg)
+{
+	struct dm_report_field_ids_type *field;
+	struct dm_list fields;
+
+	if (vg_read_error(vg)) {
+		printf("Run vg_open first\n");
+		return;
+	}
+	if (!lvm_vg_get_attr_list(vg, &fields)) {
+		printf("Error reading vg attribute list\n");
+		return;
+	}
+	printf("VG attribute names:\n");
+	dm_list_iterate_items(field, &fields) {
+		printf("%s\n", field->id);
+	}
+}
+
+static void _lvs_in_vg(vg_t *vg)
+{
+	struct dm_list *lvs;
+	struct lvm_lv_list *lvl;
+
+	if (vg_read_error(vg)) {
+		printf("Run vg_open first\n");
+		return;
+	}
+	lvs = lvm_lvs_in_vg(vg);
+	if (dm_list_empty(lvs)) {
+		printf("No LVs in VG %s\n", lvm_vg_name(vg));
+		return;
+	}
+	printf("LVs in VG %s:\n", lvm_vg_name(vg));
+	dm_list_iterate_items(lvl, lvs) {
+		printf("%s\n", lvm_lv_name(lvl->lv));
+	}
+}
+
+static void _pvs_in_vg(vg_t *vg)
+{
+	struct dm_list *pvs;
+	struct lvm_pv_list *pvl;
+
+	if (vg_read_error(vg)) {
+		printf("Run vg_open first\n");
+		return;
+	}
+	pvs = lvm_pvs_in_vg(vg);
+	if (dm_list_empty(pvs)) {
+		printf("No PVs in VG %s\n", lvm_vg_name(vg));
+		return;
+	}
+	printf("PVs in VG %s:\n", lvm_vg_name(vg));
+	dm_list_iterate_items(pvl, pvs) {
+		printf("%s\n", lvm_pv_name(pvl->pv));
+	}
+}
+
+static void _vg_get_attr_value(char **argv, int argc, vg_t *vg)
+{
+	struct dm_list fields;
+	struct dm_report_field_ids_type *field;
+	int i;
+
+	if (vg_read_error(vg)) {
+		printf("Run vg_open first\n");
+		return;
+	}
+	if (argc > 1) {
+		for (i=1; i<argc; i++)
+			get_and_print_vg_attr(vg, argv[i]);
+		return;
+	}
+	if (!lvm_vg_get_attr_list(vg, &fields)) {
+		printf("Error reading vg attribute list\n");
+		return;
+	}
+	dm_list_iterate_items(field, &fields) {
+		get_and_print_vg_attr(vg, field->id);
+	}
+}
+
+static void _lv_get_attr_list(vg_t *vg)
+{
+	struct dm_list *lvs;
+	struct lvm_lv_list *lvl;
+	struct dm_list fields;
+	struct dm_report_field_ids_type *field;
+
+	if (vg_read_error(vg)) {
+		printf("Run vg_open first\n");
+		return;
+	}
+	lvs = lvm_lvs_in_vg(vg);
+	lvl = (struct lvm_lv_list *) dm_list_first(lvs);
+	if (!lvl) {
+		printf("No LVs in VG %s\n",
+		       lvm_vg_name(vg));
+		return;
+	}
+	if (!lvm_lv_get_attr_list(lvl->lv, &fields)) {
+		printf("Error reading lv attribute list\n");
+		return;
+	}
+	printf("LV attribute names:\n");
+	dm_list_iterate_items(field, &fields) {
+		printf("%s\n", field->id);
+	}
+}
+
+static void _lv_get_attr_value(char **argv, int argc, vg_t *vg)
+{
+	struct dm_list *lvs;
+	struct lvm_lv_list *lvl;
+	struct dm_list fields;
+	struct dm_report_field_ids_type *field;
+	int i;
+
+	if (vg_read_error(vg)) {
+		printf("Run vg_open first\n");
+		return;
+	}
+	lvs = lvm_lvs_in_vg(vg);
+	lvl = (struct lvm_lv_list *) dm_list_first(lvs);
+	if (!lvl) {
+		printf("No LVs in VG %s\n",
+		       lvm_vg_name(vg));
+		return;
+	}
+	if (!lvm_lv_get_attr_list(lvl->lv, &fields)) {
+		printf("Error reading lv attribute list\n");
+		return;
+	}
+	if (argc > 1) {
+		for (i=1; i<argc; i++)
+			get_and_print_lv_attr(lvl->lv,argv[i]);
+		return;
+	}
+	dm_list_iterate_items(field, &fields) {
+		get_and_print_lv_attr(lvl->lv, field->id);
+	}
+}
+
+static void _vg_close(char **argv, int argc, vg_t **vgp)
+{
+	if (argc < 2) {
+		printf ("Please enter vg_name\n");
+		return;
+	}
+	if (!*vgp || (strcmp(argv[1], lvm_vg_name(*vgp)))) {
+		printf ("Run vg_open first\n");
+		return;
+	}
+	lvm_vg_close(*vgp);
+	*vgp = NULL;
+}	
+
 static int lvmapi_test_shell(lvm_handle_t libh)
 {
-	int argc, i;
+	int argc;
 	char *input = NULL, *args[MAX_ARGS], **argv;
+	vg_t *vg = NULL;
+	lvm_pv_obj_t *pv = NULL;
 
 	while (1) {
 		free(input);
@@ -86,12 +375,35 @@ static int lvmapi_test_shell(lvm_handle_t libh)
 			printf("Exiting.\n");
 			break;
 		} else if (!strcmp(argv[0], "?") || !strcmp(argv[0], "help")) {
-			printf("No commands defined\n");
-		} else if (!strcmp(argv[0], "scan")) {
-			for (i=1; i < argc; i++)
-				printf("Scan a path!\n");
+			_show_help();
+		} else if (!strcmp(argv[0], "vg_open")) {
+			_vg_open(argv, argc, &vg, libh);
+		} else if (!strcmp(argv[0], "vg_close")) {
+			_vg_close(argv, argc, &vg);
+		} else if (!strcmp(argv[0], "vg_get_attr_list")) {
+			_vg_get_attr_list(vg);
+		} else if (!strcmp(argv[0], "vg_get_attr_value")) {
+			_vg_get_attr_value(argv, argc, vg);
+		} else if (!strcmp(argv[0], "pvs_in_vg")) {
+			_pvs_in_vg(vg);
+		} else if (!strcmp(argv[0], "pv_obj_get")) {
+			_pv_obj_get(argv, argc, &pv, libh);
+		} else if (!strcmp(argv[0], "pv_obj_get_attr_list")) {
+			_pv_obj_get_attr_list(pv, libh);
+		} else if (!strcmp(argv[0], "pv_obj_get_attr_value")) {
+			_pv_obj_get_attr_value(argv, argc, pv);
+		} else if (!strcmp(argv[0], "lvs_in_vg")) {
+			_lvs_in_vg(vg);
+		} else if (!strcmp(argv[0], "lv_get_attr_list")) {
+			_lv_get_attr_list(vg);
+		} else if (!strcmp(argv[0], "lv_get_attr_value")) {
+			_lv_get_attr_value(argv, argc, vg);
+		} else {
+			printf ("Unrecognized command %s\n", argv[0]);
 		}
 	}
+	if (!vg_read_error(vg))
+		lvm_vg_close(vg);
 
 	free(input);
 	return 0;
-- 
1.5.5.1

[PATCH 02/13] Add lvm_create, lvm_destroy, lvm_reload_config() APIs.

[Petr Rockai]

Hi,

see comments inline.

Dave Wysochanski <dwysocha@redhat.com> writes:
> +++ b/lib/lvm2.c
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright (C) 2004-2008 Red Hat, Inc. All rights reserved.
> + *
> + * This file is part of LVM2.
> + *
> + * This copyrighted material is made available to anyone wishing to use,
> + * modify, copy, or redistribute it subject to the terms and conditions
> + * of the GNU Lesser General Public License v.2.1.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include "lvm2.h"
> +#include "lib.h"
> +#include "toolcontext.h"
> +#include "locking.h"
> +#include "metadata-exported.h"
> +#include "report.h"
> +
> +
> +lvm_handle_t lvm_create(const char *system_dir)
> +{
> +	struct cmd_context *cmd;
> +
> +	/* To be done:
> +	 * logging bound to handle
> +	 */
> +
> +	/* create context */
> +	cmd = create_toolcontext(1, system_dir);
> +
> +	/* initilize remaining */
> +	if (cmd) {
> +		int locking_type;
> +
> +		/* initilization from lvm_run_command */
> +		init_error_message_produced(0);
> +		sigint_clear();
> +
> +		/* get locking type from config tree */
> +		/* FIXME: config option needed? */
> +		locking_type = find_config_tree_int(cmd, "global/locking_type",
> +						    1);
I'm wondering if it would make sense to unify the code reading the default
locking type and the code reading the fallback options and such. As things are,
half of the configuration processing is done here and the other half inside
init_locking called below (cf lib/locking/locking.c, init_locking).

> +
> +		/* initialize locking */
> +		if (! init_locking(locking_type, cmd)) {
> +			printf("Locking type %d initialisation failed.",
> +			       locking_type);
> +			lvm_destroy((lvm_handle_t) cmd);
> +			return NULL;
> +		}
> +	}
> +
> +	return (lvm_handle_t) cmd;
I'm also wondering if the explicit cast is such a great idea afterall. (I know
this has been brought up before with my patches.) I would probably prefer to
see a comment saying /* lvm_handle_t is an alias for struct cmd_context * */ --
that way, when that fact changes, we will hopefully get a warning from the
compiler on all the places this is assumed (which might be suppressed by the
explicit cast). Or am I wrong on that count?

> +}
> +
> +
> +void lvm_destroy(lvm_handle_t libh)
> +{
> +	destroy_toolcontext((struct cmd_context *)libh);
> +	/* no error handling here */
> +}
> +
> +
> +int lvm_reload_config(lvm_handle_t libh)
> +{
> +	return refresh_toolcontext((struct cmd_context *)libh);
> +}
Should this also cater for locking re-initialisation? I suppose if
configuration changed, so might have locking.

[snip lvm2.h changes, no comments]
[snip test.c changes, no comment either]

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH 03/13] Move vg_t, lv_t, and pv_t from metadata-exported.h into lvm2.h

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:

> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  lib/lvm2.h                       |    9 +++++++++
>  lib/metadata/metadata-exported.h |    8 +-------
>  2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/lib/lvm2.h b/lib/lvm2.h
> index f348035..bfa1852 100644
> --- a/lib/lvm2.h
> +++ b/lib/lvm2.h
> @@ -60,4 +60,13 @@ void lvm_destroy(lvm_handle_t libh);
>   */
>  int lvm_reload_config(lvm_handle_t libh);
>  
> +struct volume_group;
> +typedef struct volume_group vg_t;
> +
> +struct physical_volume;
> +typedef struct physical_volume pv_t;
> +
> +struct logical_volume;
> +typedef struct logical_volume lv_t;
Did you mean to make all of those pointers? Or is the plan to expose the actual
structures instead of just handles? (I don't know, I'm just asking.)

I see you use vg_t * later on. Maybe thats' because we want to make the fact
that these are pointers explicit? Just thinking aloud...

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH 05/13] Add lvm_vg_open().

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:
> RFC - lvm_vg_open().  The new vg_read() function is a step in the right
> direction but still contains a lot of new options / flags that will
> be new to people and we can hopefully simplify.  lvm_vg_open() was
> modelled after 'open' which gives us a potential model that others are
> familiar with.  Can we use the modes and flags of 'open' in a similar way
> or do we need to introduce new flags/concepts to people?  For example,
> can we use a some of the open flags such as O_CREAT and O_EXCL to
> implement READ_CHECK_EXISTENCE?
I am currently leaning towards using a completely separate entrypoint for the
existence check function. It might have been a one bit too much flag abuse,
that one. Count it as laziness on my part.

I am not entirely convinced about the open metaphor. For one, the locking
semantics don't go well with that -- files are racy by default and need to be
quite carefully locked to prevent awful things from happening. We don't want to
make that implication. Maybe working a little on the vg_read interface would
make it acceptable to a newcomer, without causing too much confusion?

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH 06/13] Add lvm_vg_close().

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:
> +void lvm_vg_close(vg_t *vg)
> +{
> +	if (vgname_is_locked(vg->name))
> +		unlock_vg(vg->cmd, vg->name);
> +}
Do we want to do reference counting in here? Might be moot if we don't allow
multiple opens. But contrary to what I said on IRC last time, it might be worth
to have proper lock upgrading procedure, and it would be sort of nice if that
could be achieved (for the user) by just calling the open function again on the
same VG, with write permission request. One use case is automated recovery,
which could be probably reformulated elegantly in terms of a lock upgrade.

Another is the one you have mentioned, "read in the vg, wibble around a little
and then decide you want to change it". The solution I proposed back then, to
close and open for writing again, is inherently race-prone, so you would need
to do all your wibbling again. Of course, the lock upgrade might fail, in which
case, you will have to anyway, but maybe it still makes the API a little more
friendly and less tempting to do the wrong thing.

However, now I think about it, even that probably doesn't need refcounting if
we structure the lock upgrade API accordingly. Ok, clear.

I'm getting pretty tangential now, but another thought: should be the locking
state encapsulated in the library handle, as opposed to process-global? This
probably depends whether the locks can actually prevent the same process (in
say different thread) from acquiring the lock second time (ie. whether they are
effective in preventing unintended concurrent accesses from same process). If
they are, it wouldn't be so hard to make the proposed lvmlib API thread-safe,
by requiring that each thread owns its own library handle. This might be of
direct benefit to eg. dmeventd, which currently has a fairly ugly mutex hack
instead.

Sorry for the tangential reply...

Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH 08/13] Add lvm_vg_get_attr_value() libLVM API to query to value of a VG attribute.

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:
> This API may be used independently to query the value of a VG attribute,
> provided the name of the attribute is known.  If the name of the attribute(s)
> are unknown, the previous API, vg_get_attr_list(), may be used.
I suppose this makes the field names part of the API. Nothing wrong with that,
they are already exported into the command-level interface, just wanted to make
that fact explicit.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH 12/13] First cut at adding pv_obj_* APIs.

[Petr Rockai]

Hi,

Dave Wysochanski <dwysocha@redhat.com> writes:
> Impelemnt pv_obj_get(), pv_obj_get_list(), pv_obj_get_attr_list() and
> pv_obj_get_attr_value().
>
> This is a first attempt at adding pv_obj_* APIs as outlined by
> http://fedoraproject.org/wiki/LVM/liblvm/api
>
> Why add another structure when we could use struct physical_volume
> and other internal structs?  At this point it is not clear but would
> provide us with the ability to more easily allocate and free handles,
> and would allow places to store things like the access mode and API
> errors without polluting the internal structures.
I don't say this is completely wrong, but the naming seems very baroque to me,
and not at all clear what it means, in relation to pv_t, lv_t and such. Also,
how does this relate to the idea that we want to push back the concept of a PV
as away from users as possible? I think Alasdair mentioned that on several
occasions -- PVs should become implicit, as opposed to explicit. Although it
might be a stretch for library. Not sure. What do you think?

As for internal structures, I suppose the major hurdle is the way we allocate
those in the toolcontext pool (and then maybe their complex interlinked
nature). I wouldn't say that another layer of, especially
heap-or-pool-allocated indirection will do any good. I think we already have
more than is good for us: the lvmcache is keeping a shadow copy of everything
already in an incompatible format from the struct volume_group and
friends. This happens to be the on-disk text format in a string buffer. I
believe this is also due to allocation and linking issues of the current struct
volume_group representation. So improving the situation with the struct
volume_group etc. would probably help the lvmcache, as well as the lvmlib
API. Maybe that would be a worthy investment then, instead of building up
another incompatible shadow structure?

Also, the patch seems to be duplicating a lot of code from previous patches? Or
were those for different kind of object (listing and checking attributes, eg.)?

[snip patch]

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH] Move locking_type reading inside init_locking().

[Dave Wysochanski]

No functional change.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/locking/locking.c |    5 +++++
 tools/lvmcmdline.c    |    3 +--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/locking/locking.c b/lib/locking/locking.c
index b5871ae..bf28d9a 100644
--- a/lib/locking/locking.c
+++ b/lib/locking/locking.c
@@ -208,11 +208,16 @@ static void _update_vg_lock_count(uint32_t flags)
 
 /*
  * Select a locking type
+ * type: locking type; if < 0, then read config tree value
  */
 int init_locking(int type, struct cmd_context *cmd)
 {
 	init_lockingfailed(0);
 
+	if (type < 0)
+		type = find_config_tree_int(cmd, "global/locking_type", 1);
+		
+	
 	switch (type) {
 	case 0:
 		init_no_locking(&_locking, cmd);
diff --git a/tools/lvmcmdline.c b/tools/lvmcmdline.c
index 2dabe64..bafc6a5 100644
--- a/tools/lvmcmdline.c
+++ b/tools/lvmcmdline.c
@@ -995,8 +995,7 @@ int lvm_run_command(struct cmd_context *cmd, int argc, char **argv)
 	if (arg_count(cmd, nolocking_ARG))
 		locking_type = 0;
 	else
-		locking_type = find_config_tree_int(cmd,
-					       "global/locking_type", 1);
+		locking_type = -1;
 
 	if (!init_locking(locking_type, cmd)) {
 		log_error("Locking type %d initialisation failed.",
-- 
1.6.0.5

[PATCH take2] Add lvm_create, lvm_destroy, lvm_reload_config() APIs.

[Dave Wysochanski]

Add lvm2.c file.
Update test code.
Call init_locking() with locking_type = -1 to read config file value.

Signed-off-by: Thomas Woerner <twoerner@redhat.com>
Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/Makefile.in |    3 +-
 lib/lvm2.c      |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/lvm2.h      |   44 +++++++++++++++++++++++--------------
 test/api/test.c |   13 +++++------
 4 files changed, 99 insertions(+), 25 deletions(-)
 create mode 100644 lib/lvm2.c

diff --git a/lib/Makefile.in b/lib/Makefile.in
index 54092cd..c0b58b9 100644
--- a/lib/Makefile.in
+++ b/lib/Makefile.in
@@ -86,7 +86,8 @@ SOURCES =\
 	report/report.c \
 	striped/striped.c \
 	uuid/uuid.c \
-	zero/zero.c
+	zero/zero.c \
+	lvm2.c
 
 ifeq ("@LVM1@", "internal")
   SOURCES +=\
diff --git a/lib/lvm2.c b/lib/lvm2.c
new file mode 100644
index 0000000..ef1ef21
--- /dev/null
+++ b/lib/lvm2.c
@@ -0,0 +1,64 @@
+/*
+ * Copyright (C) 2004-2008 Red Hat, Inc. All rights reserved.
+ *
+ * This file is part of LVM2.
+ *
+ * This copyrighted material is made available to anyone wishing to use,
+ * modify, copy, or redistribute it subject to the terms and conditions
+ * of the GNU Lesser General Public License v.2.1.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include "lvm2.h"
+#include "lib.h"
+#include "toolcontext.h"
+#include "locking.h"
+#include "metadata-exported.h"
+#include "report.h"
+
+
+lvm_handle_t lvm_create(const char *system_dir)
+{
+	struct cmd_context *cmd;
+
+	/* To be done:
+	 * logging bound to handle
+	 */
+
+	/* create context */
+	cmd = create_toolcontext(1, system_dir);
+
+	/* initilize remaining */
+	if (cmd) {
+		/* initilization from lvm_run_command */
+		init_error_message_produced(0);
+		sigint_clear();
+
+		/* FIXME: locking_type config option needed? */
+		/* initialize locking */
+		if (! init_locking(-1, cmd)) {
+			printf("Locking type %d initialisation failed.",
+			       locking_type);
+			lvm_destroy((lvm_handle_t) cmd);
+			return NULL;
+		}
+	}
+
+	return (lvm_handle_t) cmd;
+}
+
+
+void lvm_destroy(lvm_handle_t libh)
+{
+	destroy_toolcontext((struct cmd_context *)libh);
+	/* no error handling here */
+}
+
+
+int lvm_reload_config(lvm_handle_t libh)
+{
+	return refresh_toolcontext((struct cmd_context *)libh);
+}
diff --git a/lib/lvm2.h b/lib/lvm2.h
index b18cead..f348035 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -18,36 +18,46 @@
 #include <stdint.h>
 
 /*
- * Library Initialisation
- * FIXME: For now just #define lvm2_create() and lvm2_destroy() to 
- * create_toolcontext() and destroy_toolcontext()
+ * lvm_handle_t
  */
-struct arg;
-struct cmd_context;
-struct cmd_context *create_toolcontext(unsigned is_long_lived);
-void destroy_toolcontext(struct cmd_context *cmd);
+struct lib_context; /* internal context data */
+typedef struct lib_context *lvm_handle_t;
 
+#ifdef FUTURE_FEATURE
 /*
- * lvm2_create
-lvm_handle_t lvm2_create(void);
+ * lvm_config_t
+ */
+struct lvm_config {
+	const char *system_dir;
+	unsigned is_long_lived;
+};
+#endif /* FUTURE_FEATURE */
+
+/*
+ * lvm_create
  *
- * Description: Create an LVM2 handle used in many other APIs.
+ * Description: Create an LVM handle used in many other APIs.
  *
  * Returns:
  * NULL: Fail - unable to initialise handle.
- * non-NULL: Success - valid LVM2 handle returned
+ * non-NULL: Success - valid LVM handle returned
  */
-#define lvm2_create(X) create_toolcontext(1)
+lvm_handle_t lvm_create(const char *system_dir);
 
 /*
- * lvm2_destroy
-void lvm2_destroy(lvm_handle_t h);
+ * lvm_destroy
  *
- * Description: Destroy an LVM2 handle allocated with lvm2_create
+ * Description: Destroy an LVM handle allocated with lvm_create
  *
  * Parameters:
- * - h (IN): handle obtained from lvm2_create
+ * - h (IN): handle obtained from lvm_create
+ */
+void lvm_destroy(lvm_handle_t libh);
+
+/*
+ * lvm_reload_config
+ * Reload configuration files
  */
-#define lvm2_destroy(X) destroy_toolcontext(X)
+int lvm_reload_config(lvm_handle_t libh);
 
 #endif
diff --git a/test/api/test.c b/test/api/test.c
index de53c46..9411663 100644
--- a/test/api/test.c
+++ b/test/api/test.c
@@ -48,7 +48,7 @@ static int lvm_split(char *str, int *argc, char **argv, int max)
 	return *argc;
 }
 
-static int lvmapi_test_shell(void *h)
+static int lvmapi_test_shell(lvm_handle_t libh)
 {
 	int argc, i;
 	char *input = NULL, *args[MAX_ARGS], **argv;
@@ -99,18 +99,17 @@ static int lvmapi_test_shell(void *h)
 		      
 int main (int argc, char *argv[])
 {
-	void *h;
+	lvm_handle_t libh;
 
-	h = lvm2_create();
-	if (!h) {
+	libh = lvm_create(NULL);
+	if (!libh) {
 		printf("Unable to open lvm library instance\n");
 		return 1;
 	}
 
-	lvmapi_test_shell(h);
+	lvmapi_test_shell(libh);
 
-	if (h)
-		lvm2_destroy(h);
+	lvm_destroy(libh);
 	return 0;
 }
 
-- 
1.6.0.5

[PATCH 03/13] Move vg_t, lv_t, and pv_t from metadata-exported.h into lvm2.h

[Dave Wysochanski]

On Tue, 2009-02-03 at 00:25 +0100, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> 
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > ---
> >  lib/lvm2.h                       |    9 +++++++++
> >  lib/metadata/metadata-exported.h |    8 +-------
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/lvm2.h b/lib/lvm2.h
> > index f348035..bfa1852 100644
> > --- a/lib/lvm2.h
> > +++ b/lib/lvm2.h
> > @@ -60,4 +60,13 @@ void lvm_destroy(lvm_handle_t libh);
> >   */
> >  int lvm_reload_config(lvm_handle_t libh);
> >  
> > +struct volume_group;
> > +typedef struct volume_group vg_t;
> > +
> > +struct physical_volume;
> > +typedef struct physical_volume pv_t;
> > +
> > +struct logical_volume;
> > +typedef struct logical_volume lv_t;
> Did you mean to make all of those pointers? Or is the plan to expose the actual
> structures instead of just handles? (I don't know, I'm just asking.)
> 
> I see you use vg_t * later on. Maybe thats' because we want to make the fact
> that these are pointers explicit? Just thinking aloud...
> 

The plan is not to expose the structs, just needed to declare the
handles.

[PATCH 05/13] Add lvm_vg_open().

[Dave Wysochanski]

On Tue, 2009-02-03 at 00:28 +0100, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> > RFC - lvm_vg_open().  The new vg_read() function is a step in the right
> > direction but still contains a lot of new options / flags that will
> > be new to people and we can hopefully simplify.  lvm_vg_open() was
> > modelled after 'open' which gives us a potential model that others are
> > familiar with.  Can we use the modes and flags of 'open' in a similar way
> > or do we need to introduce new flags/concepts to people?  For example,
> > can we use a some of the open flags such as O_CREAT and O_EXCL to
> > implement READ_CHECK_EXISTENCE?
> I am currently leaning towards using a completely separate entrypoint for the
> existence check function. It might have been a one bit too much flag abuse,
> that one. Count it as laziness on my part.
> 
> I am not entirely convinced about the open metaphor. For one, the locking
> semantics don't go well with that -- files are racy by default and need to be
> quite carefully locked to prevent awful things from happening. We don't want to
> make that implication. Maybe working a little on the vg_read interface would
> make it acceptable to a newcomer, without causing too much confusion?
>

Well, I'm not convinced of either direction really.  At this point I
think I will continue down the open path and see where I get stuck.
Refining your vg_read() patches is good - both efforts should lead to a
better interface in the long run, whatever we choose.

[PATCH 06/13] Add lvm_vg_close().

[Dave Wysochanski]

On Tue, 2009-02-03 at 00:45 +0100, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> > +void lvm_vg_close(vg_t *vg)
> > +{
> > +	if (vgname_is_locked(vg->name))
> > +		unlock_vg(vg->cmd, vg->name);
> > +}
> Do we want to do reference counting in here? Might be moot if we don't allow
> multiple opens. But contrary to what I said on IRC last time, it might be worth
> to have proper lock upgrading procedure, and it would be sort of nice if that
> could be achieved (for the user) by just calling the open function again on the
> same VG, with write permission request. One use case is automated recovery,
> which could be probably reformulated elegantly in terms of a lock upgrade.
> 

Well, calling the open function again on the same VG would be a new
interface that we'd have to explain so I'd lean against it.  Is there an
example elsewhere we could point at?  If we went this route we'd need to
keep a list of handles internally (we may end up needing this anyway for
true handle validation) and then just pass back the same one.

Also I think Thomas was opposed to the lock upgrading.


> Another is the one you have mentioned, "read in the vg, wibble around a little
> and then decide you want to change it". The solution I proposed back then, to
> close and open for writing again, is inherently race-prone, so you would need
> to do all your wibbling again. Of course, the lock upgrade might fail, in which
> case, you will have to anyway, but maybe it still makes the API a little more
> friendly and less tempting to do the wrong thing.
> 

Right - we'll need to weigh pros and cons.  Might need to prototype each
approach and see what we find.


> However, now I think about it, even that probably doesn't need refcounting if
> we structure the lock upgrade API accordingly. Ok, clear.
> 
> I'm getting pretty tangential now, but another thought: should be the locking
> state encapsulated in the library handle, as opposed to process-global? This
> probably depends whether the locks can actually prevent the same process (in
> say different thread) from acquiring the lock second time (ie. whether they are
> effective in preventing unintended concurrent accesses from same process). If
> they are, it wouldn't be so hard to make the proposed lvmlib API thread-safe,
> by requiring that each thread owns its own library handle. This might be of
> direct benefit to eg. dmeventd, which currently has a fairly ugly mutex hack
> instead.
> 
By lock state, do you mean the type of locking, the locks on the
open/reads, or both?  The open/read mode we'd of course have to tie to
the vg handles.  

Does it makes sense to allow 2 threads to call init_locking() with
different lock types?

Like other areas the locking code needs work if we're serious about
thread safety.

[PATCH 06/13] Add lvm_vg_close().

[Dave Wysochanski]

On Wed, 2009-02-04 at 12:11 -0500, Dave Wysochanski wrote:
> On Tue, 2009-02-03 at 00:45 +0100, Petr Rockai wrote:
> > Dave Wysochanski <dwysocha@redhat.com> writes:
> > > +void lvm_vg_close(vg_t *vg)
> > > +{
> > > +	if (vgname_is_locked(vg->name))
> > > +		unlock_vg(vg->cmd, vg->name);
> > > +}
> > Do we want to do reference counting in here? Might be moot if we don't allow
> > multiple opens. But contrary to what I said on IRC last time, it might be worth
> > to have proper lock upgrading procedure, and it would be sort of nice if that
> > could be achieved (for the user) by just calling the open function again on the
> > same VG, with write permission request. One use case is automated recovery,
> > which could be probably reformulated elegantly in terms of a lock upgrade.
> > 
> 
> Well, calling the open function again on the same VG would be a new
> interface that we'd have to explain so I'd lean against it.  Is there an
> example elsewhere we could point at?  If we went this route we'd need to
> keep a list of handles internally (we may end up needing this anyway for
> true handle validation) and then just pass back the same one.
> 
> Also I think Thomas was opposed to the lock upgrading.
> 

There are plenty of examples of lock upgrading though normally it's on
some sort of lock call, not an open.  I know we said we wanted to hide
the locking - maybe still possible.  But if we need lock upgrading it
might be clearer if we exposed a lock API directly.  *gasp*

[PATCH 12/13] First cut at adding pv_obj_* APIs.

[Dave Wysochanski]

On Tue, 2009-02-03 at 00:58 +0100, Petr Rockai wrote:
> Hi,
> 
> Dave Wysochanski <dwysocha@redhat.com> writes:
> > Impelemnt pv_obj_get(), pv_obj_get_list(), pv_obj_get_attr_list() and
> > pv_obj_get_attr_value().
> >
> > This is a first attempt at adding pv_obj_* APIs as outlined by
> > http://fedoraproject.org/wiki/LVM/liblvm/api
> >
> > Why add another structure when we could use struct physical_volume
> > and other internal structs?  At this point it is not clear but would
> > provide us with the ability to more easily allocate and free handles,
> > and would allow places to store things like the access mode and API
> > errors without polluting the internal structures.
> I don't say this is completely wrong, but the naming seems very baroque to me,
> and not at all clear what it means, in relation to pv_t, lv_t and such. Also,
> how does this relate to the idea that we want to push back the concept of a PV
> as away from users as possible? I think Alasdair mentioned that on several
> occasions -- PVs should become implicit, as opposed to explicit. Although it
> might be a stretch for library. Not sure. What do you think?
> 

We discussed removing the PV concept from the API earlier this week on a
concall.  From my notes/recollection, here is what we concluded:
1. We should aim to ensure a PV is always in a VG, even if it is just
the orphan VG.
2. To obtain a PV object, you must first obtain a VG object.
3. If you want to change a PV attribute, you must first do a
vg_read/vg_open and obtain WRITE permission.
Thomas is going to update the API document.

I agree it is not desirable to introduce another set of structures
unless there is good reason for it.  It was an initial implementation
and something Thomas and I were discussing.  The reason we were leaning
towards a new structure was we needed a place to store the options for
the create functions (e.g. pvcreate, vgcreate, lvcreate).  In some cases
these are attributes of the actual PV, VG, or LV.  But in other cases
they are options to control the create behavior.  We should probably
think about each of the options specifically and see what the best way
to handle them.  I know this wasn't apparent from my patchset here but
as I recall this is why we were adding a new structure.


> As for internal structures, I suppose the major hurdle is the way we allocate
> those in the toolcontext pool (and then maybe their complex interlinked
> nature). I wouldn't say that another layer of, especially
> heap-or-pool-allocated indirection will do any good. I think we already have
> more than is good for us: the lvmcache is keeping a shadow copy of everything
> already in an incompatible format from the struct volume_group and
> friends. This happens to be the on-disk text format in a string buffer. I
> believe this is also due to allocation and linking issues of the current struct
> volume_group representation. So improving the situation with the struct
> volume_group etc. would probably help the lvmcache, as well as the lvmlib
> API. Maybe that would be a worthy investment then, instead of building up
> another incompatible shadow structure?
> 

Agreed the internal structures are a bit confusing with various
partially done abstractions, etc.  If we can clean them up it would be
great.

> Also, the patch seems to be duplicating a lot of code from previous patches? Or
> were those for different kind of object (listing and checking attributes, eg.)?
> 

Probably so - patches were just a first stab.

Thanks for the comments.

[PATCH 08/13] Add lvm_vg_get_attr_value() libLVM API to query to value of a VG attribute.

[Dave Wysochanski]

On Tue, 2009-02-03 at 00:47 +0100, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> > This API may be used independently to query the value of a VG attribute,
> > provided the name of the attribute is known.  If the name of the attribute(s)
> > are unknown, the previous API, vg_get_attr_list(), may be used.
> I suppose this makes the field names part of the API. Nothing wrong with that,
> they are already exported into the command-level interface, just wanted to make
> that fact explicit.
> 

Yes.  The field names will be the same as in the reporting commands of
the tools so in theory, these should be well-known names and should not
change.

Note that this implementation of lvm_vg_get_attr_value() and
lvm_vg_get_attr_list() is deprecated by my latest attempt at attributes
based on the dm_report_output_attributes() function.  My second attempt
was meant to address the following 2 shortcomings:
1. The report handle must be kept alive as long as the attributes are
used.  To accomplish this, I allocate the report handle inside vg_open()
and deallocate inside vg_close()
2. There was no good way to match up a field name with a value other
than repeatedly calling lvm_vg_get_attr_value().  To solve this I
decided a better approach was to define a attribute structure, which
contained the name and the value.  This will also allow me to add an
access mode for the attribute (read-only or read/write).

[PATCH 02/13] Add lvm_create, lvm_destroy, lvm_reload_config() APIs.

[Dave Wysochanski]

On Tue, 2009-02-03 at 00:22 +0100, Petr Rockai wrote:
> Hi,
> 
> see comments inline.
> 
> Dave Wysochanski <dwysocha@redhat.com> writes:
> > +++ b/lib/lvm2.c
> > @@ -0,0 +1,70 @@
> > +/*
> > + * Copyright (C) 2004-2008 Red Hat, Inc. All rights reserved.
> > + *
> > + * This file is part of LVM2.
> > + *
> > + * This copyrighted material is made available to anyone wishing to use,
> > + * modify, copy, or redistribute it subject to the terms and conditions
> > + * of the GNU Lesser General Public License v.2.1.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public License
> > + * along with this program; if not, write to the Free Software Foundation,
> > + * Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> > + */
> > +
> > +#include "lvm2.h"
> > +#include "lib.h"
> > +#include "toolcontext.h"
> > +#include "locking.h"
> > +#include "metadata-exported.h"
> > +#include "report.h"
> > +
> > +
> > +lvm_handle_t lvm_create(const char *system_dir)
> > +{
> > +	struct cmd_context *cmd;
> > +
> > +	/* To be done:
> > +	 * logging bound to handle
> > +	 */
> > +
> > +	/* create context */
> > +	cmd = create_toolcontext(1, system_dir);
> > +
> > +	/* initilize remaining */
> > +	if (cmd) {
> > +		int locking_type;
> > +
> > +		/* initilization from lvm_run_command */
> > +		init_error_message_produced(0);
> > +		sigint_clear();
> > +
> > +		/* get locking type from config tree */
> > +		/* FIXME: config option needed? */
> > +		locking_type = find_config_tree_int(cmd, "global/locking_type",
> > +						    1);
> I'm wondering if it would make sense to unify the code reading the default
> locking type and the code reading the fallback options and such. As things are,
> half of the configuration processing is done here and the other half inside
> init_locking called below (cf lib/locking/locking.c, init_locking).
> 

I did submit a patch I thought was a simple cleanup but more work is
needed to clean up locking.  Ideally I'd like to place init_locking
inside create_toolcontext and just make this the initialization
function.  I ran into a problem with clvmd though being a "locking
implementor" or "locking backend".  Alasdair spent some time with me
brainstorming how we might solve this - splitting the API into different
portions:
1. Calls ok for a locking back-end
2. Calls ok for a locking front-end
3. Calls ok for both

I have temporarily put this on the back-burner while I try to solve some
other things but should make another attempt at cleanup soon.


> > +
> > +void lvm_destroy(lvm_handle_t libh)
> > +{
> > +	destroy_toolcontext((struct cmd_context *)libh);
> > +	/* no error handling here */
> > +}
> > +
> > +
> > +int lvm_reload_config(lvm_handle_t libh)
> > +{
> > +	return refresh_toolcontext((struct cmd_context *)libh);
> > +}
> Should this also cater for locking re-initialisation? I suppose if
> configuration changed, so might have locking.
> 

Indeed - thanks.