[PATCH] 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] Use struct cmd_context* as type for lvm2_handle_t with hidden cmd_context. Accessors and mutators have to be used. (lib/lvm2.h)

[Thomas Woerner]

---
 lib/lvm2.h      |   23 +++++++++++------------
 test/api/test.c |   13 ++++++-------
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/lib/lvm2.h b/lib/lvm2.h
index 8ddaf68..b131fd7 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -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
diff --git a/test/api/test.c b/test/api/test.c
index de53c46..866be8f 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(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;
 }
 
-- 
1.6.0.4

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

[Thomas Woerner]

---
 lib/commands/toolcontext.c |   25 +++++++++++++++++++++----
 lib/commands/toolcontext.h |    1 +
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/lib/commands/toolcontext.c b/lib/commands/toolcontext.c
index e9ef66c..e37af6e 100644
--- a/lib/commands/toolcontext.c
+++ b/lib/commands/toolcontext.c
@@ -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;
diff --git a/lib/commands/toolcontext.h b/lib/commands/toolcontext.h
index 4ea3486..a37911f 100644
--- a/lib/commands/toolcontext.h
+++ b/lib/commands/toolcontext.h
@@ -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);
-- 
1.6.0.4

[PATCH] New lib/lvm2.c for base library functions

[Thomas Woerner]

---
 lib/Makefile.in |    3 ++-
 lib/lvm2.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 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..daf9a79
--- /dev/null
+++ b/lib/lvm2.c
@@ -0,0 +1,42 @@
+/*
+ * 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"
+
+
+lvm2_handle_t lvm2_create(const char *sys_dir)
+{
+  struct cmd_context *cmd;
+
+  /* use internal version with system_dir */
+  cmd = create_librarycontext(sys_dir);
+  if (! cmd)
+    return NULL;
+
+  /* TODO:
+   * - bind logging to handle
+   */
+
+  return (lvm2_handle_t) cmd;
+}
+
+
+void lvm2_destroy(lvm2_handle_t libh)
+{
+  destroy_toolcontext((struct cmd_context *) libh); /* no error handling here */
+
+  return 1;
+}
-- 
1.6.0.4

[PATCH] New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

[Thomas Woerner]

---
 lib/lvm2.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/lvm2.h |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 0 deletions(-)

diff --git a/lib/lvm2.c b/lib/lvm2.c
index daf9a79..ac5e583 100644
--- a/lib/lvm2.c
+++ b/lib/lvm2.c
@@ -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;
+}
diff --git a/lib/lvm2.h b/lib/lvm2.h
index b131fd7..b2c97d0 100644
--- a/lib/lvm2.h
+++ b/lib/lvm2.h
@@ -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
-- 
1.6.0.4

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

[Dave Wysochanski]

On Thu, 2008-12-11 at 17:43 +0100, Thomas Woerner wrote:
> ---
>  lib/lvm2.h      |   23 +++++++++++------------
>  test/api/test.c |   13 ++++++-------
>  2 files changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/lvm2.h b/lib/lvm2.h
> index 8ddaf68..b131fd7 100644
> --- a/lib/lvm2.h
> +++ b/lib/lvm2.h
> @@ -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);

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

>  
>  #endif
> diff --git a/test/api/test.c b/test/api/test.c
> index de53c46..866be8f 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(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;
>  }
>  

Looks ok.

[PATCH] New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

[Dave Wysochanski]

On Thu, 2008-12-11 at 17:43 +0100, Thomas Woerner wrote:
> ---
>  lib/lvm2.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/lvm2.h |   38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 90 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/lvm2.c b/lib/lvm2.c
> index daf9a79..ac5e583 100644
> --- a/lib/lvm2.c
> +++ b/lib/lvm2.c
> @@ -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?

> +
> +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;
> +}
>  

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

> 
>  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);
> +

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).


> +  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;
> +}
> diff --git a/lib/lvm2.h b/lib/lvm2.h
> index b131fd7..b2c97d0 100644
> --- a/lib/lvm2.h
> +++ b/lib/lvm2.h
> @@ -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);
> +

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?

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

[Alasdair G Kergon]

On Thu, Dec 11, 2008 at 05:43:39PM +0100, Thomas Woerner wrote:
> diff --git a/lib/lvm2.h b/lib/lvm2.h
> +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
-- 
agk at redhat.com

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

[Alasdair G Kergon]

On Thu, Dec 11, 2008 at 05:43:40PM +0100, Thomas Woerner wrote:
> +++ b/lib/commands/toolcontext.c
> @@ -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!

> +	if (sys_dir)
> +		strncpy(cmd->sys_dir, sys_dir, PATH_MAX);

Oops - trailing NULL if it's too long?

> @@ -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)
> +{
> +	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
-- 
agk at redhat.com

[PATCH] New lib/lvm2.c for base library functions

[Alasdair G Kergon]

On Thu, Dec 11, 2008 at 05:43:41PM +0100, Thomas Woerner wrote:
>  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.

> +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.)

> +void lvm2_destroy(lvm2_handle_t libh)
...
> +  return 1;

:-)

Alasdair
-- 
agk at redhat.com

[PATCH] New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

[Alasdair G Kergon]

On Thu, Dec 11, 2008 at 04:49:55PM -0500, Dave Wysochanski wrote:
> On Thu, 2008-12-11 at 17:43 +0100, Thomas Woerner wrote:
> > +++ b/lib/lvm2.c
> > @@ -15,6 +15,30 @@
> > +#include "../tools/tools.h"
> Do we need the relative path here?

Indeed, that's a no-no.

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

> > @@ -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.

> > +int lvm2_reload_config(lvm2_handle_t libh)

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

> > +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.)

> > + * 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)

> > +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
-- 
agk at redhat.com

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

[Dave Wysochanski]

On Fri, 2008-12-12 at 01:04 +0000, Alasdair G Kergon wrote:
> > +/* 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);
> > +}
> 
> 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?

[PATCH] New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

[Thomas Woerner]

Dave Wysochanski wrote:
> On Thu, 2008-12-11 at 17:43 +0100, Thomas Woerner wrote:
>> ---
>>  lib/lvm2.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/lvm2.h |   38 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 90 insertions(+), 0 deletions(-)
>>
>> diff --git a/lib/lvm2.c b/lib/lvm2.c
>> index daf9a79..ac5e583 100644
>> --- a/lib/lvm2.c
>> +++ b/lib/lvm2.c
>> @@ -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".

>> +
>> +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;
>> +}
>>  
> 
> We should avoid duplicating this code with lvmcmdline.c.  Did you check
> the other callers to see if we could consolidate somehow?
> 
>>  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);
>> +
> 
> 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).
> 
> 
>> +  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;
>> +}
>> diff --git a/lib/lvm2.h b/lib/lvm2.h
>> index b131fd7..b2c97d0 100644
>> --- a/lib/lvm2.h
>> +++ b/lib/lvm2.h
>> @@ -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);
>> +
> 
> 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


-- 
Thomas Woerner
Software Engineer            Phone: +49-711-96437-310
Red Hat GmbH                 Fax  : +49-711-96437-111
Hauptstaetterstr. 58         Email: Thomas Woerner <twoerner@redhat.com>
D-70178 Stuttgart            Web  : http://www.redhat.de/

[PATCH] New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

[Alasdair G Kergon]

On Fri, Dec 12, 2008 at 12:36:30PM +0100, Thomas Woerner wrote:
> 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
-- 
agk at redhat.com

[PATCH] New public functions lvm2_reload_config, lvm2_set_config_option and lvm2_reset_config_option.

[Dave Wysochanski]

On Fri, 2008-12-12 at 12:36 +0100, Thomas Woerner wrote:
> >> diff --git a/lib/lvm2.c b/lib/lvm2.c
> >> index daf9a79..ac5e583 100644
> >> --- a/lib/lvm2.c
> >> +++ b/lib/lvm2.c
> >> @@ -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".

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.