Skip to contentSkip to conversation

Use struct cmd_context* as type for lvm2_handle_t with hidden cmd_context. Accessors and mutators have to be used. (lib/lvm2.h)

PATCH New Thomas Woerner 3 participants 14 messages Dec 11, 2008 – Dec 12, 2008 Library code

lore
Share & tools
lore

Thomas Woerner

author
#0 lore raw

PATCH 1/1 Use struct cmd_context* as type for lvm2_handle_t with hidden cmd_context. Accessors and mutators have to be used. (lib/lvm2.h)

---
 lib/lvm2.h      |   23 +++++++++++------------
 test/api/test.c |   13 ++++++-------
 2 files changed, 17 insertions(+), 19 deletions(-)
lib/lvm2.h +11 -12
index 8ddaf68..b131fd7 100644
@@ -1,5 +1,4 @@
 /*
- * Copyright (C) 2001-2004 Sistina Software, Inc. All rights reserved.  
  * Copyright (C) 2004-2008 Red Hat, Inc. All rights reserved.
  *
  * This file is part of LVM2.

@@ -17,37 +16,37 @@
 
 #include <stdint.h>
 
+/*** Library Initialisation ***/
+
 /*
- * Library Initialisation
- * FIXME: For now just #define lvm2_create() and lvm2_destroy() to 
- * create_toolcontext() and destroy_toolcontext()
+ * lvm2_handle_t
  */
-struct arg;
-struct cmd_context;
-struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static, unsigned is_long_lived);
-void destroy_toolcontext(struct cmd_context *cmd);
+struct cmd_context; /* private context */
+typedef struct cmd_context* lvm2_handle_t;
 
 /*
  * lvm2_create
-lvm_handle_t lvm2_create(void);
  *
  * Description: Create an LVM2 handle used in many other APIs.
  *
+ * Parameters:
+ * - sys_dir: Directory containing lvm.conf and other LVM system files,
+ *            overwritten by LVM_SYSTEM_DIR environment variable (if set)
+ *
  * Returns:
  * NULL: Fail - unable to initialise handle.
  * non-NULL: Success - valid LVM2 handle returned
  */
-#define lvm2_create(X) create_toolcontext(NULL,0,1)
+lvm2_handle_t lvm2_create(const char *sys_dir);
 
 /*
  * lvm2_destroy
-void lvm2_destroy(lvm_handle_t h);
  *
  * Description: Destroy an LVM2 handle allocated with lvm2_create
  *
  * Parameters:
  * - h (IN): handle obtained from lvm2_create
  */
-#define lvm2_destroy(X) destroy_toolcontext(X)
+void lvm2_destroy(lvm2_handle_t h);
 
 #endif
test/api/test.c +6 -7
index de53c46..866be8f 100644
@@ -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(lvm2_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;
+	lvm2_handle_t libh;
 
-	h = lvm2_create();
-	if (!h) {
+	libh = lvm2_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);
+	lvm2_destroy(libh);
 	return 0;
 }
Signature · 1 line
1.6.0.4
author
#1 lore raw

PATCH 1/1 Renamed create_toolcontext function to _create_context and added sys_dir as argument, new functions create_librarycontext and create_toolcontext.

---
 lib/commands/toolcontext.c |   25 +++++++++++++++++++++----
 lib/commands/toolcontext.h |    1 +
 2 files changed, 22 insertions(+), 4 deletions(-)
lib/commands/toolcontext.c +21 -4
index e9ef66c..e37af6e 100644
@@ -990,9 +990,10 @@ static void _init_rand(struct cmd_context *cmd)
 	cmd->rand_seed = (unsigned) time(NULL) + (unsigned) getpid();
 }
 
-/* Entry point */
-struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
-				       unsigned is_long_lived)
+static struct cmd_context *_create_context(struct arg *the_args,
+					   unsigned is_static,
+					   unsigned is_long_lived,
+					   const char *sys_dir)
 {
 	struct cmd_context *cmd;
 

@@ -1024,7 +1025,10 @@ struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
 	dm_list_init(&cmd->tags);
 	dm_list_init(&cmd->config_files);
 
-	strcpy(cmd->sys_dir, DEFAULT_SYS_DIR);
+	strncpy(cmd->sys_dir, DEFAULT_SYS_DIR, PATH_MAX);
+	
+	if (sys_dir)
+		strncpy(cmd->sys_dir, sys_dir, PATH_MAX);
 
 	if (!_get_env_vars(cmd))
 		goto error;

@@ -1101,6 +1105,19 @@ struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
 	return NULL;
 }
 
+/* Library Entry point */
+struct cmd_context *create_librarycontext(const char *sys_dir)
+{
+	return _create_context(NULL, 0, 1, sys_dir);
+}
+
+/* Tool Entry point */
+struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
+				       unsigned is_long_lived)
+{
+	return _create_context(the_args, is_static, is_long_lived, NULL);
+}
+
 static void _destroy_formats(struct dm_list *formats)
 {
 	struct dm_list *fmtl, *tmp;
lib/commands/toolcontext.h +1 -0
index 4ea3486..a37911f 100644
@@ -96,6 +96,7 @@ struct cmd_context {
 	char sysfs_dir[PATH_MAX];
 };
 
+struct cmd_context *create_librarycontext(const char *sys_dir);
 struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static, unsigned is_long_lived);
 void destroy_toolcontext(struct cmd_context *cmd);
 int refresh_toolcontext(struct cmd_context *cmd);
Signature · 1 line
1.6.0.4
author
#2 lore raw

PATCH 1/1 New lib/lvm2.c for base library functions

---
 lib/Makefile.in |    3 ++-
 lib/lvm2.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletions(-)
 create mode 100644 lib/lvm2.c
lib/Makefile.in +2 -1
index 54092cd..c0b58b9 100644
@@ -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 +=\
lib/lvm2.c new file +42 -0

42 lines not on this page · raw message

Signature · 1 line
1.6.0.4
author
#3 lore raw

PATCH 1/1 New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

---
 lib/lvm2.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/lvm2.h |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 0 deletions(-)
lib/lvm2.c +52 -0
index daf9a79..ac5e583 100644
@@ -15,6 +15,30 @@
 #include "lvm2.h"
 #include "lib.h"
 #include "toolcontext.h"
+#include "archiver.h"
+#include "activate.h"
+#include "../tools/tools.h"
+
+
+static void _apply_settings(struct cmd_context *cmd)
+{
+  init_debug(cmd->current_settings.debug);
+  init_verbose(cmd->current_settings.verbose + VERBOSE_BASE_LEVEL);
+  init_test(cmd->current_settings.test);
+  init_full_scan_done(0);
+  init_mirror_in_sync(0);
+  
+  init_msg_prefix(cmd->default_settings.msg_prefix);
+  init_cmd_name(cmd->default_settings.cmd_name);
+
+  archive_enable(cmd, cmd->current_settings.archive);
+  backup_enable(cmd, cmd->current_settings.backup);
+
+  set_activation(cmd->current_settings.activation);
+
+  cmd->fmt = cmd->current_settings.fmt;
+  cmd->handles_missing_pvs = 0;
+}
 
 
 lvm2_handle_t lvm2_create(const char *sys_dir)

@@ -30,6 +54,8 @@ lvm2_handle_t lvm2_create(const char *sys_dir)
    * - bind logging to handle
    */
 
+  _apply_settings(cmd);
+
   return (lvm2_handle_t) cmd;
 }
 

@@ -40,3 +66,29 @@ void lvm2_destroy(lvm2_handle_t libh)
 
   return 1;
 }
+
+
+int lvm2_reload_config(lvm2_handle_t libh)
+{
+  int refresh;
+  
+  refresh = refresh_toolcontext((struct cmd_context *) libh);
+
+  if (refresh)
+    _apply_settings((struct cmd_context *) libh);
+
+  return refresh;
+}
+
+
+int lvm2_set_config_option(lvm2_handle_t libh, const char *option,
+			   const char *value)
+{
+  return 1;
+}
+
+
+int lvm2_reset_config_option(lvm2_handle_t libh, const char *option)
+{
+  return 1;
+}
lib/lvm2.h +38 -0
index b131fd7..b2c97d0 100644
@@ -49,4 +49,42 @@ lvm2_handle_t lvm2_create(const char *sys_dir);
  */
 void lvm2_destroy(lvm2_handle_t h);
 
+/*
+ * lvm2_reload_config
+ *
+ * Description: Reload configuration files
+ *
+ * Returns:
+ *   0: error
+ *   1: success
+ */
+int lvm2_reload_config(lvm2_handle_t h);
+
+/*
+ * lvm2_set_config_option
+ *
+ * Description: Load an lvm config option into the existing configuration.
+ *   The formation of the option parameter is similar to the names
+ *   in /etc/lvm/lvm.conf.
+ *   An option within a section is specified with a '/' between the
+ *   section name and option.  For example, the 'filter' option in the
+ *   devices section is specified by 'devices/filter'
+ *
+ * Returns:
+ *   0: error
+ *   1: success
+ */
+int lvm2_set_config_option(lvm2_handle_t h, const char *option,
+			   const char *value);
+/*
+ * lvm2_remove_config_option
+ *
+ * Description:
+ *
+ * Returns:
+ *   0: error
+ *   1: success
+ */
+int lvm2_reset_config_option(lvm2_handle_t h, const char *option);
+
 #endif
Signature · 1 line
1.6.0.4
#4 lore raw

PATCH 1/1 New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

Quoted 10 lines · Thomas Woerner
---
 lib/lvm2.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/lvm2.h |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 0 deletions(-)
lib/lvm2.c +0 -0
index daf9a79..ac5e583 100644

lib/lvm2.c line 21

Quoted 7 lines
lib/lvm2.c +4 -0
@@ -15,6 +15,30 @@
 #include "lvm2.h"
 #include "lib.h"
 #include "toolcontext.h"
+#include "archiver.h"
+#include "activate.h"
+#include "../tools/tools.h"
+

Do we need the relative path here?

Quoted 11 lines
lib/lvm2.c +11 -0
+
+static void _apply_settings(struct cmd_context *cmd)
+{
+  init_debug(cmd->current_settings.debug);
+  init_verbose(cmd->current_settings.verbose + VERBOSE_BASE_LEVEL);
+  init_test(cmd->current_settings.test);
+  init_full_scan_done(0);
+  init_mirror_in_sync(0);
+  
+  init_msg_prefix(cmd->default_settings.msg_prefix);
+  init_cmd_name(cmd->default_settings.cmd_name);

lib/lvm2.c line 42

Quoted 10 lines
lib/lvm2.c +9 -0
+
+  archive_enable(cmd, cmd->current_settings.archive);
+  backup_enable(cmd, cmd->current_settings.backup);
+
+  set_activation(cmd->current_settings.activation);
+
+  cmd->fmt = cmd->current_settings.fmt;
+  cmd->handles_missing_pvs = 0;
+}

We should avoid duplicating this code with lvmcmdline.c. Did you check the other callers to see if we could consolidate somehow?

Quoted 13 lines
lib/lvm2.c +2 -0
 lvm2_handle_t lvm2_create(const char *sys_dir)

@@ -30,6 +54,8 @@ lvm2_handle_t lvm2_create(const char *sys_dir)
    * - bind logging to handle
    */
 
+  _apply_settings(cmd);
+
   return (lvm2_handle_t) cmd;
 }
 

@@ -40,3 +66,29 @@ void lvm2_destroy(lvm2_handle_t libh)

lib/lvm2.c void lvm2_destroy(lvm2_handle_t libh) · line 76

Quoted 10 lines
lib/lvm2.c +8 -0
   return 1;
 }
+
+
+int lvm2_reload_config(lvm2_handle_t libh)
+{
+  int refresh;
+  
+  refresh = refresh_toolcontext((struct cmd_context *) libh);
+

I think we need:
cmd->current_settings = cmd->default_settings;

either here or at the bottom of refresh_toolcontext(). This I think is a bug in the existing code so it does not necessarily apply to this patch. It is a problem for callers of refresh_toolcontext() though, and I believe clvmd has this problem (see do_refresh_cache() in lvm-functions.c).

Quoted 56 lines
lib/lvm2.c +18 -0
+  if (refresh)
+    _apply_settings((struct cmd_context *) libh);
+
 
 
+  return refresh;
+}
+
+
+int lvm2_set_config_option(lvm2_handle_t libh, const char *option,
+			   const char *value)
+{
+  return 1;
+}
+
+
+int lvm2_reset_config_option(lvm2_handle_t libh, const char *option)
+{
+  return 1;
+}
lib/lvm2.h +28 -0
index b131fd7..b2c97d0 100644
@@ -49,4 +49,42 @@ lvm2_handle_t lvm2_create(const char *sys_dir);
  */
 void lvm2_destroy(lvm2_handle_t h);
 
+/*
+ * lvm2_reload_config
+ *
+ * Description: Reload configuration files
+ *
+ * Returns:
+ *   0: error
+ *   1: success
+ */
+int lvm2_reload_config(lvm2_handle_t h);
+
+/*
+ * lvm2_set_config_option
+ *
+ * Description: Load an lvm config option into the existing configuration.
+ *   The formation of the option parameter is similar to the names
+ *   in /etc/lvm/lvm.conf.
+ *   An option within a section is specified with a '/' between the
+ *   section name and option.  For example, the 'filter' option in the
+ *   devices section is specified by 'devices/filter'
+ *
+ * Returns:
+ *   0: error
+ *   1: success
+ */
+int lvm2_set_config_option(lvm2_handle_t h, const char *option,
+			   const char *value);
+/*

lib/lvm2.h lvm2_handle_t lvm2_create(const char *sys_dir); · line 89

Quoted 10 lines
lib/lvm2.h +10 -0
+ * lvm2_remove_config_option
+ *
+ * Description:
+ *
+ * Returns:
+ *   0: error
+ *   1: success
+ */
+int lvm2_reset_config_option(lvm2_handle_t h, const char *option);
+

We should probably be consistent with return code types - uint32_t?

Then again, if we're going to try to use errno return values perhaps we need int after all?

#5 lore raw

PATCH 1/1 New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

Quoted 5 lines · Dave Wysochanski

lib/lvm2.c line 15

Quoted 3 lines · Thomas Woerner

+++ b/lib/lvm2.c

lib/lvm2.c +1 -0
@@ -15,6 +15,30 @@
+#include "../tools/tools.h"

Do we need the relative path here?

Indeed, that's a no-no.

Quoted 2 lines

We should avoid duplicating this code with lvmcmdline.c. Did you check the other callers to see if we could consolidate somehow?

Ditto.

Quoted 2 lines

lib/lvm2.c lvm2_handle_t lvm2_create(const char *sys_dir) · line 54

Quoted 2 lines
lib/lvm2.c +1 -0
@@ -30,6 +54,8 @@ lvm2_handle_t lvm2_create(const char *sys_dir)
+  _apply_settings(cmd);

Superfluous, due to the recent set of checkins from Dave.

Quoted 1 line

lib/lvm2.c void lvm2_destroy(lvm2_handle_t libh) · line 71

Quoted 1 line
lib/lvm2.c +1 -0
+int lvm2_reload_config(lvm2_handle_t libh)

Again, these functions should be integrated into the library proper.

Quoted 7 lines

lib/lvm2.c void lvm2_destroy(lvm2_handle_t libh) · line 91

Quoted 7 lines
lib/lvm2.c +7 -0
+int lvm2_set_config_option(lvm2_handle_t libh, const char *option,
+			   const char *value)
+{
+  return 1;
+}
+
+int lvm2_reset_config_option(lvm2_handle_t libh, const char *option)

Do we really need that yet?
How about just exposing a function that clears them all in one go for now? To reset individual ones we could use the previous function with a value of NULL as meaning to delete it from the tree.
(Arguably an option of NULL could mean to reset them all.)

Quoted 6 lines
Quoted 6 lines

+ * Description: Load an lvm config option into the existing configuration.

+ *   The formation of the option parameter is similar to the names
+ *   in /etc/lvm/lvm.conf.
+ *   An option within a section is specified with a '/' between the
+ *   section name and option.  For example, the 'filter' option in the
+ *   devices section is specified by 'devices/filter'

We do already handle that format to some extent:

# lvm dumpconfig log/verbose
verbose=0
# lvm dumpconfig log/file
file="/var/log/lvm2.log"

so let's try to extend the generic functionality to handle it?

Perhaps:

# lvm dumpconfig --config 'log/verbose=3' log/verbose
verbose=3

with a new cmdline option to make that output
log/verbose=3

(and as this is generic, it would also work in lvm.conf of course)

Quoted 7 lines

lib/lvm2.h lvm2_handle_t lvm2_create(const char *sys_dir); · line 88

Quoted 5 lines
lib/lvm2.h +4 -0
+int lvm2_set_config_option(lvm2_handle_t h, const char *option,
+			   const char *value);
 
+ * lvm2_remove_config_option
+int lvm2_reset_config_option(lvm2_handle_t h, const char *option);

We should probably be consistent with return code types - uint32_t?

Userspace library - int should be adequate, don't think there's a need for fixed-width is there?

Alasdair

Signature · 1 line
agk at redhat.com
author
#6 lore raw

PATCH 1/1 New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

In reply to Dave Wysochanski
Quoted 6 lines · Dave Wysochanski
Quoted 5 lines · Thomas Woerner
---
 lib/lvm2.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/lvm2.h |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 0 deletions(-)
Quoted 15 lines

lib/lvm2.c line 21

Quoted 12 lines
lib/lvm2.c +4 -0
index daf9a79..ac5e583 100644
@@ -15,6 +15,30 @@
 #include "lvm2.h"
 #include "lib.h"
 #include "toolcontext.h"
+#include "archiver.h"
+#include "activate.h"
+#include "../tools/tools.h"
+

Do we need the relative path here?

Ok, I will add tools/tools.h, tools/toollib.h, tools/commands.h and tools/args.h to include/.smylinks. Then it is possible to use #include "tools.h".

Quoted 134 lines
Quoted 11 lines
lib/lvm2.c +11 -0
+
+static void _apply_settings(struct cmd_context *cmd)
+{
+  init_debug(cmd->current_settings.debug);
+  init_verbose(cmd->current_settings.verbose + VERBOSE_BASE_LEVEL);
+  init_test(cmd->current_settings.test);
+  init_full_scan_done(0);
+  init_mirror_in_sync(0);
+  
+  init_msg_prefix(cmd->default_settings.msg_prefix);
+  init_cmd_name(cmd->default_settings.cmd_name);

lib/lvm2.c line 42

Quoted 10 lines
lib/lvm2.c +9 -0
+
+  archive_enable(cmd, cmd->current_settings.archive);
+  backup_enable(cmd, cmd->current_settings.backup);
+
+  set_activation(cmd->current_settings.activation);
+
+  cmd->fmt = cmd->current_settings.fmt;
+  cmd->handles_missing_pvs = 0;
+}

We should avoid duplicating this code with lvmcmdline.c. Did you check the other callers to see if we could consolidate somehow?

Quoted 12 lines
lib/lvm2.c +2 -0
 lvm2_handle_t lvm2_create(const char *sys_dir)

@@ -30,6 +54,8 @@ lvm2_handle_t lvm2_create(const char *sys_dir)
    * - bind logging to handle
    */
 
+  _apply_settings(cmd);
+
   return (lvm2_handle_t) cmd;
 }
 

@@ -40,3 +66,29 @@ void lvm2_destroy(lvm2_handle_t libh)

lib/lvm2.c void lvm2_destroy(lvm2_handle_t libh) · line 76

Quoted 10 lines
lib/lvm2.c +8 -0
   return 1;
 }
+
+
+int lvm2_reload_config(lvm2_handle_t libh)
+{
+  int refresh;
+  
+  refresh = refresh_toolcontext((struct cmd_context *) libh);
+

I think we need:
cmd->current_settings = cmd->default_settings;

either here or at the bottom of refresh_toolcontext(). This I think is a bug in the existing code so it does not necessarily apply to this patch. It is a problem for callers of refresh_toolcontext() though, and I believe clvmd has this problem (see do_refresh_cache() in lvm-functions.c).

Quoted 56 lines
lib/lvm2.c +18 -0
+  if (refresh)
+    _apply_settings((struct cmd_context *) libh);
+
 
 
+  return refresh;
+}
+
+
+int lvm2_set_config_option(lvm2_handle_t libh, const char *option,
+			   const char *value)
+{
+  return 1;
+}
+
+
+int lvm2_reset_config_option(lvm2_handle_t libh, const char *option)
+{
+  return 1;
+}
lib/lvm2.h +28 -0
index b131fd7..b2c97d0 100644
@@ -49,4 +49,42 @@ lvm2_handle_t lvm2_create(const char *sys_dir);
  */
 void lvm2_destroy(lvm2_handle_t h);
 
+/*
+ * lvm2_reload_config
+ *
+ * Description: Reload configuration files
+ *
+ * Returns:
+ *   0: error
+ *   1: success
+ */
+int lvm2_reload_config(lvm2_handle_t h);
+
+/*
+ * lvm2_set_config_option
+ *
+ * Description: Load an lvm config option into the existing configuration.
+ *   The formation of the option parameter is similar to the names
+ *   in /etc/lvm/lvm.conf.
+ *   An option within a section is specified with a '/' between the
+ *   section name and option.  For example, the 'filter' option in the
+ *   devices section is specified by 'devices/filter'
+ *
+ * Returns:
+ *   0: error
+ *   1: success
+ */
+int lvm2_set_config_option(lvm2_handle_t h, const char *option,
+			   const char *value);
+/*

lib/lvm2.h lvm2_handle_t lvm2_create(const char *sys_dir); · line 89

Quoted 10 lines
lib/lvm2.h +10 -0
+ * lvm2_remove_config_option
+ *
+ * Description:
+ *
+ * Returns:
+ *   0: error
+ *   1: success
+ */
+int lvm2_reset_config_option(lvm2_handle_t h, const char *option);
+

We should probably be consistent with return code types - uint32_t?

Then again, if we're going to try to use errno return values perhaps we need int after all?

--
lvm-devel mailing list
lvm-devel at redhat.com
https://www.redhat.com/mailman/listinfo/lvm-devel

Signature · 5 lines
Thomas Woerner
Software Engineer            Phone: +49-711-96437-310
Red Hat GmbH                 Fax  : +49-711-96437-111
Hauptstaetterstr. 58         Email: Thomas Woerner <[email protected]>
D-70178 Stuttgart            Web  : http://www.redhat.de/
#7 lore raw

PATCH 1/1 New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

Quoted 3 lines · Thomas Woerner

Ok, I will add tools/tools.h, tools/toollib.h, tools/commands.h and tools/args.h to include/.smylinks. Then it is possible to use #include "tools.h".

You can't use *anything* from the tools directory in the library! That would be like linking the library against anaconda or other client code!
Where you need any functions in there you need to move them first into the lib directory behind a proper interface.

Alasdair

Signature · 1 line
agk at redhat.com
#8 lore raw

PATCH 1/1 New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

In reply to Thomas Woerner
Quoted 15 lines · Thomas Woerner
Quoted 6 lines
Quoted 6 lines
lib/lvm2.c +0 -0
index daf9a79..ac5e583 100644
@@ -15,6 +15,30 @@
 #include "lvm2.h"
Quoted 9 lines

lib/lvm2.c line 21

Quoted 6 lines
lib/lvm2.c +4 -0
 #include "lib.h"
 #include "toolcontext.h"
+#include "archiver.h"
+#include "activate.h"
+#include "../tools/tools.h"
+

Do we need the relative path here?

Quoted 4 lines

Ok, I will add tools/tools.h, tools/toollib.h, tools/commands.h and tools/args.h to include/.smylinks. Then it is possible to use #include
"tools.h".

Actually, including tools/*.h header files in a library file is not good. If we need to do that we should probably try to refactor something.

#9 lore raw

PATCH 1/1 New lib/lvm2.c for base library functions

In reply to Thomas Woerner
Quoted 1 line · Thomas Woerner
 lib/lvm2.c      |   42 ++++++++++++++++++++++++++++++++++++++++++

I'd prefer not to have a 'special' file like that. Please place the functions elsewhere in the directory hierachy according to what they are doing.

lib/lvm2.c line 25

Quoted 6 lines
lib/lvm2.c new file +6 -0
+lvm2_handle_t lvm2_create(const char *sys_dir)
+{
+  struct cmd_context *cmd;
+
+  /* use internal version with system_dir */
+  cmd = create_librarycontext(sys_dir);

No need for this wrapper at this stage - just use the main function directly! (And make sure the files in the tools directory use these functions as part of the same patch.)

Quoted 1 line

+void lvm2_destroy(lvm2_handle_t libh)

...

Quoted 1 line

+ return 1;

:-)

Alasdair

Signature · 1 line
agk at redhat.com
#10 lore raw

PATCH 1/1 Renamed create_toolcontext function to _create_context and added sys_dir as argument, new functions create_librarycontext and create_toolcontext.

In reply to Thomas Woerner

lib/commands/toolcontext.c struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static, · line 1025

Quoted 13 lines · Thomas Woerner

+++ b/lib/commands/toolcontext.c

lib/commands/toolcontext.c +5 -3
@@ -990,9 +990,10 @@ static void _init_rand(struct cmd_context *cmd)
 
-struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
-				       unsigned is_long_lived)
+static struct cmd_context *_create_context(struct arg *the_args,
+					   unsigned is_static,
+					   unsigned is_long_lived,
+					   const char *sys_dir)
 

@@ -1024,7 +1025,10 @@ struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
-	strcpy(cmd->sys_dir, DEFAULT_SYS_DIR);
+	strncpy(cmd->sys_dir, DEFAULT_SYS_DIR, PATH_MAX);

No need for that one - I think it's safe to assume DEFAULT_SYS_DIR is shorter than PATH_MAX or else it would be impossible to use!

lib/commands/toolcontext.c struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static, · line 1031

Quoted 2 lines
lib/commands/toolcontext.c +2 -0
+	if (sys_dir)
+		strncpy(cmd->sys_dir, sys_dir, PATH_MAX);

Oops - trailing NULL if it's too long?

Quoted 4 lines
lib/commands/toolcontext.c +2 -0
@@ -1101,6 +1105,19 @@ struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
 
+/* Library Entry point */
+struct cmd_context *create_librarycontext(const char *sys_dir)

lib/commands/toolcontext.c struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static, · line 1117

Quoted 10 lines
lib/commands/toolcontext.c +9 -0
+{
+	return _create_context(NULL, 0, 1, sys_dir);
+}
 
+/* Tool Entry point */
+struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
+				       unsigned is_long_lived)
+{
+	return _create_context(the_args, is_static, is_long_lived, NULL);
+}

Please don't separate these. All tools will use the same library interface - there is to be nothing privileged about the existing tools in the tools dir or anaconda - they will both share the same interface. So work out where those parameters should live.

Alasdair

Signature · 1 line
agk at redhat.com
#11 lore raw

PATCH 1/1 Renamed create_toolcontext function to _create_context and added sys_dir as argument, new functions create_librarycontext and create_toolcontext.

Quoted 6 lines · Alasdair G Kergon
Quoted 5 lines
lib/commands/toolcontext.c +5 -0
+/* Library Entry point */
+struct cmd_context *create_librarycontext(const char *sys_dir)
+{
+	return _create_context(NULL, 0, 1, sys_dir);
+}
Quoted 12 lines

lib/commands/toolcontext.c struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static, · line 1119

Quoted 6 lines
lib/commands/toolcontext.c +6 -0
+/* Tool Entry point */
+struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
+				       unsigned is_long_lived)
+{
+	return _create_context(the_args, is_static, is_long_lived, NULL);
+}

Please don't separate these. All tools will use the same library interface - there is to be nothing privileged about the existing tools in the tools dir or anaconda - they will both share the same interface. So work out where those parameters should live.

If we don't separate tool from library init, then we make generic callers of the library incur the penalty for the tools (e.g. explicitly setting args = NULL, etc). Or is there an alternative?

Going with the object theme, and along the lines of what Thomas (and maybe others) have suggested for pvcreate, perhaps a 'struct lvm_lib_obj' that gets created with default values (such as args == NULL) and you have to explicitly set what you want before passing in the lib_config object to create_toolcontext? Passing NULL there of course gives you default values as well. Would that be a more acceptable direction?

#12 lore raw

PATCH 1/1 Use struct cmd_context* as type for lvm2_handle_t with hidden cmd_context. Accessors and mutators have to be used. (lib/lvm2.h)

In reply to Thomas Woerner
Quoted 52 lines · Thomas Woerner
---
 lib/lvm2.h      |   23 +++++++++++------------
 test/api/test.c |   13 ++++++-------
 2 files changed, 17 insertions(+), 19 deletions(-)
lib/lvm2.h +10 -10
index 8ddaf68..b131fd7 100644
@@ -1,5 +1,4 @@
 /*
- * Copyright (C) 2001-2004 Sistina Software, Inc. All rights reserved.  
  * Copyright (C) 2004-2008 Red Hat, Inc. All rights reserved.
  *
  * This file is part of LVM2.

@@ -17,37 +16,37 @@
 
 #include <stdint.h>
 
+/*** Library Initialisation ***/
+
 /*
- * Library Initialisation
- * FIXME: For now just #define lvm2_create() and lvm2_destroy() to 
- * create_toolcontext() and destroy_toolcontext()
+ * lvm2_handle_t
  */
-struct arg;
-struct cmd_context;
-struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static, unsigned is_long_lived);
-void destroy_toolcontext(struct cmd_context *cmd);
+struct cmd_context; /* private context */
+typedef struct cmd_context* lvm2_handle_t;
 
 /*
  * lvm2_create
-lvm_handle_t lvm2_create(void);
  *
  * Description: Create an LVM2 handle used in many other APIs.
  *
+ * Parameters:
+ * - sys_dir: Directory containing lvm.conf and other LVM system files,
+ *            overwritten by LVM_SYSTEM_DIR environment variable (if set)
+ *
  * Returns:
  * NULL: Fail - unable to initialise handle.
  * non-NULL: Success - valid LVM2 handle returned
  */
-#define lvm2_create(X) create_toolcontext(NULL,0,1)
+lvm2_handle_t lvm2_create(const char *sys_dir);
 
 /*

lib/lvm2.h line 50

Quoted 10 lines
lib/lvm2.h +1 -2
  * lvm2_destroy
-void lvm2_destroy(lvm_handle_t h);
  *
  * Description: Destroy an LVM2 handle allocated with lvm2_create
  *
  * Parameters:
  * - h (IN): handle obtained from lvm2_create
  */
-#define lvm2_destroy(X) destroy_toolcontext(X)
+void lvm2_destroy(lvm2_handle_t h);

Might have been nice to leave the #defines until the later patch when you actually add these functions but no big deal.

Quoted 29 lines
lib/lvm2.h +0 -0
 #endif
test/api/test.c +4 -4
index de53c46..866be8f 100644
@@ -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(lvm2_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;
+	lvm2_handle_t libh;
 
-	h = lvm2_create();
-	if (!h) {
+	libh = lvm2_create(NULL);
+	if (!libh) {
 		printf("Unable to open lvm library instance\n");
 		return 1;
 	}

test/api/test.c static int lvmapi_test_shell(void *h) · line 115

Quoted 10 lines
test/api/test.c +2 -3
 
-	lvmapi_test_shell(h);
+	lvmapi_test_shell(libh);
 
-	if (h)
-		lvm2_destroy(h);
+	lvm2_destroy(libh);
 	return 0;
 }

Looks ok.

#13 lore raw

PATCH 1/1 Use struct cmd_context* as type for lvm2_handle_t with hidden cmd_context. Accessors and mutators have to be used. (lib/lvm2.h)

In reply to Thomas Woerner

lib/lvm2.h line 50

Quoted 2 lines · Thomas Woerner
lib/lvm2.h +1 -0
+void lvm2_destroy(lvm2_handle_t h);

Let's use a longer variable name than 'h' and something unique so we can grep for it easily.

Apart from that, ACK.

Alasdair

Signature · 1 line
agk at redhat.com