[PATCH 0/5] Fixes

[PATCH 0/5] Fixes

[Zdenek Kabelac]

Mostly minor sign cast warning fixes 
and not yet confirmed setvbuf update.

Zdenek Kabelac (5):
  change API to use unsigned for #copies
  Cast to 'int'
  Fix sign warning
  Ignore dm_snprintf result
  Set our own in/out buffers

 daemons/clvmd/lvm-functions.c                      |    2 +-
 .../dmeventd/plugins/snapshot/dmeventd_snapshot.c  |    4 +-
 lib/commands/toolcontext.c                         |   30 +++++++++++++++++++-
 lib/commands/toolcontext.h                         |    4 ++-
 lib/filters/filter.h                               |    2 +-
 lib/log/log.c                                      |    4 +-
 lib/metadata/metadata-exported.h                   |    2 +-
 lib/metadata/metadata.c                            |    2 +-
 liblvm/lvm_base.c                                  |    2 +-
 tools/lvmcmdline.c                                 |    2 +-
 10 files changed, 42 insertions(+), 12 deletions(-)

-- 
1.7.4.4

[PATCH 1/5] change API to use unsigned for #copies

[Zdenek Kabelac]

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/metadata/metadata-exported.h |    2 +-
 lib/metadata/metadata.c          |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 80de552a..dac155a 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -428,7 +428,7 @@ struct physical_volume *pv_create(const struct cmd_context *cmd,
 				  uint32_t existing_extent_count,
 				  uint32_t existing_extent_size,
 				  uint64_t label_sector,
-				  int pvmetadatacopies,
+				  unsigned pvmetadatacopies,
 				  uint64_t pvmetadatasize,
 				  unsigned metadataignore);
 int pv_resize(struct physical_volume *pv, struct volume_group *vg,
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 47a2d98..756ce41 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1582,7 +1582,7 @@ struct physical_volume *pv_create(const struct cmd_context *cmd,
 				  uint32_t existing_extent_count,
 				  uint32_t existing_extent_size,
 				  uint64_t label_sector,
-				  int pvmetadatacopies,
+				  unsigned pvmetadatacopies,
 				  uint64_t pvmetadatasize,
 				  unsigned metadataignore)
 {
-- 
1.7.4.4

[PATCH 2/5] Cast to 'int'

[Zdenek Kabelac]

As we use both - unsigned and signed ints to decode major number - cast
it to 'int' to avoid warnings here.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/filters/filter.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/filters/filter.h b/lib/filters/filter.h
index 07611f9..f63f8c2 100644
--- a/lib/filters/filter.h
+++ b/lib/filters/filter.h
@@ -21,7 +21,7 @@
 #include <sys/stat.h>
 
 #ifdef linux
-#  define MAJOR(dev)	((dev & 0xfff00) >> 8)
+#  define MAJOR(dev)	((int)(dev & 0xfff00) >> 8)
 #  define MINOR(dev)	((dev & 0xff) | ((dev >> 12) & 0xfff00))
 #  define MKDEV(ma,mi)	((mi & 0xff) | (ma << 8) | ((mi & ~0xff) << 12))
 #else
-- 
1.7.4.4

[PATCH 2/5] Cast to 'int'

[Milan Broz]

On 04/19/2011 02:55 PM, Zdenek Kabelac wrote:

>  #ifdef linux
> -#  define MAJOR(dev)	((dev & 0xfff00) >> 8)
> +#  define MAJOR(dev)	((int)(dev & 0xfff00) >> 8)
>  #  define MINOR(dev)	((dev & 0xff) | ((dev >> 12) & 0xfff00))
>  #  define MKDEV(ma,mi)	((mi & 0xff) | (ma << 8) | ((mi & ~0xff) << 12))
>  #else

I we really want to play this cast games (seems I was infected too:-),
please fix all defines to use (int) - like MAJOR() so it is consistent
despite there is no current gcc warning.

Milan
p.s.
or use gnu_dev_major()/minor, it is reliable these days.

[PATCH 3/5] Fix sign warning

[Zdenek Kabelac]

Funny one -  'minor()' and 'major()'  should return 'int' according to
its man page.  But they are mapped to their gnu_dev friends.

i.e.
/usr/include/sys/sysmacros.h:

  extern unsigned int gnu_dev_minor (unsigned long long int __dev)

  #ifdef __GLIBC_HAVE_LONG_LONG
     # define minor(dev) gnu_dev_minor (dev)
  #endif

To keep warning quiet - cast to the 'expected' int.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 .../dmeventd/plugins/snapshot/dmeventd_snapshot.c  |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/daemons/dmeventd/plugins/snapshot/dmeventd_snapshot.c b/daemons/dmeventd/plugins/snapshot/dmeventd_snapshot.c
index 2b09670..788fd90 100644
--- a/daemons/dmeventd/plugins/snapshot/dmeventd_snapshot.c
+++ b/daemons/dmeventd/plugins/snapshot/dmeventd_snapshot.c
@@ -162,8 +162,8 @@ static void _umount(const char *device, int major, int minor)
 			continue; /* can't stat, skip this one */
 
 		if (S_ISBLK(st.st_mode) &&
-		    major(st.st_rdev) == major &&
-		    minor(st.st_rdev) == minor) {
+		    (int) major(st.st_rdev) == major &&
+		    (int) minor(st.st_rdev) == minor) {
 			syslog(LOG_ERR, "Unmounting invalid snapshot %s from %s.", device, words[1]);
                         if (!_run(UMOUNT_COMMAND, "-fl", words[1], NULL))
                                 syslog(LOG_ERR, "Failed to umount snapshot %s from %s: %s.",
-- 
1.7.4.4

[PATCH 4/5] Ignore dm_snprintf result

[Zdenek Kabelac]

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/log/log.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/log/log.c b/lib/log/log.c
index ff98cdd..96ac474 100644
--- a/lib/log/log.c
+++ b/lib/log/log.c
@@ -275,8 +275,8 @@ void print_log(int level, const char *file, int line, int dm_errno,
       log_it:
 	if (!_log_suppress) {
 		if (verbose_level() > _LOG_DEBUG)
-			dm_snprintf(locn, sizeof(locn), "#%s:%d ",
-				     file, line);
+			(void) dm_snprintf(locn, sizeof(locn), "#%s:%d ",
+					   file, line);
 		else
 			locn[0] = '\0';
 
-- 
1.7.4.4

[PATCH 5/5] Set our own in/out buffers

[Zdenek Kabelac]

Memory lock from critical_section is now being kept over the critical
section - mallopt() should ensure, that mmap is not used for allocation,
and we preallocate some memory to be able to satisfy some small
alloc request. However when glibc needs buffers for line buffering of
input and output buffers - it allocates these buffers in such way it
adds memory page for each such buffer and size of unlock memory check will
mismatch by 1 or 2 pages.

This happens when we print or read lines without '\n' so these buffers
are used. To avoid this extra allocation, use setvbuf to set these
bufffers ahead.

Buffer are preallocated by the create_toolcontext flag.
For clvmd we keep the original glibc settings.

FIXME: Bigger fix is needs to avoid quering users while holding VG
locks, as such question might take quite some time....
Thus should in effect unlock memory before displaying such message.
But some more work needs to be done for this - so hack for now.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 daemons/clvmd/lvm-functions.c |    2 +-
 lib/commands/toolcontext.c    |   30 +++++++++++++++++++++++++++++-
 lib/commands/toolcontext.h    |    4 +++-
 liblvm/lvm_base.c             |    2 +-
 tools/lvmcmdline.c            |    2 +-
 5 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/daemons/clvmd/lvm-functions.c b/daemons/clvmd/lvm-functions.c
index 166cd7f..8e68ed9 100644
--- a/daemons/clvmd/lvm-functions.c
+++ b/daemons/clvmd/lvm-functions.c
@@ -919,7 +919,7 @@ int init_clvm(int using_gulm, char **argv)
 	init_syslog(LOG_DAEMON);
 	openlog("clvmd", LOG_PID, LOG_DAEMON);
 
-	if (!(cmd = create_toolcontext(1, NULL))) {
+	if (!(cmd = create_toolcontext(1, NULL, 0))) {
 		log_error("Failed to allocate command context");
 		return 0;
 	}
diff --git a/lib/commands/toolcontext.c b/lib/commands/toolcontext.c
index 30c8fc6..c2a8568 100644
--- a/lib/commands/toolcontext.c
+++ b/lib/commands/toolcontext.c
@@ -58,6 +58,8 @@
 #  include <malloc.h>
 #endif
 
+static const size_t linebuffer_size = 4096;
+
 static int _get_env_vars(struct cmd_context *cmd)
 {
 	const char *e;
@@ -1127,7 +1129,8 @@ static void _init_globals(struct cmd_context *cmd)
 
 /* Entry point */
 struct cmd_context *create_toolcontext(unsigned is_long_lived,
-				       const char *system_dir)
+				       const char *system_dir,
+				       unsigned set_buffering)
 {
 	struct cmd_context *cmd;
 
@@ -1162,6 +1165,22 @@ struct cmd_context *create_toolcontext(unsigned is_long_lived,
 	/* FIXME Make this configurable? */
 	reset_lvm_errno(1);
 
+	/* Set in/out stream buffering before glibc */
+	if (set_buffering) {
+		/* Allocate 2 buffers */
+		if (!(cmd->linebuffer = dm_malloc(2 * linebuffer_size))) {
+			log_error("Failed to allocate line buffer.");
+			goto out;
+		}
+		if ((setvbuf(stdin, cmd->linebuffer, _IOLBF, linebuffer_size) ||
+		     setvbuf(stdout, cmd->linebuffer + linebuffer_size,
+			     _IOLBF, linebuffer_size))) {
+			log_sys_error("setvbuf", "");
+			goto out;
+		}
+		/* Buffers are used for lines without '\n' */
+	}
+
 	/*
 	 * Environment variable LVM_SYSTEM_DIR overrides this below.
 	 */
@@ -1398,6 +1417,15 @@ void destroy_toolcontext(struct cmd_context *cmd)
 	_destroy_tag_configs(cmd);
 	if (cmd->libmem)
 		dm_pool_destroy(cmd->libmem);
+
+	if (cmd->linebuffer) {
+		/* Reset stream buffering to defaults */
+		setlinebuf(stdin);
+		fflush(stdout);
+		setlinebuf(stdout);
+		dm_free(cmd->linebuffer);
+	}
+
 	dm_free(cmd);
 
 	release_log_memory();
diff --git a/lib/commands/toolcontext.h b/lib/commands/toolcontext.h
index 4628c7c..1c3368e 100644
--- a/lib/commands/toolcontext.h
+++ b/lib/commands/toolcontext.h
@@ -66,6 +66,7 @@ struct cmd_context {
 	const char *kernel_vsn;
 
 	unsigned rand_seed;
+	char *linebuffer;
 	const char *cmd_line;
 	struct command *command;
 	char **argv;
@@ -109,7 +110,8 @@ struct cmd_context {
  * The environment variable LVM_SYSTEM_DIR always takes precedence.
  */
 struct cmd_context *create_toolcontext(unsigned is_long_lived,
-				       const char *system_dir);
+				       const char *system_dir,
+				       unsigned set_buffering);
 void destroy_toolcontext(struct cmd_context *cmd);
 int refresh_toolcontext(struct cmd_context *cmd);
 int refresh_filters(struct cmd_context *cmd);
diff --git a/liblvm/lvm_base.c b/liblvm/lvm_base.c
index 9e1a8ec..f44b786 100644
--- a/liblvm/lvm_base.c
+++ b/liblvm/lvm_base.c
@@ -34,7 +34,7 @@ lvm_t lvm_init(const char *system_dir)
 	/* create context */
 	/* FIXME: split create_toolcontext */
 	/* FIXME: make all globals configurable */
-	cmd = create_toolcontext(0, system_dir);
+	cmd = create_toolcontext(0, system_dir, 1);
 	if (!cmd)
 		return NULL;
 
diff --git a/tools/lvmcmdline.c b/tools/lvmcmdline.c
index ddf0b5e..04f6a5c 100644
--- a/tools/lvmcmdline.c
+++ b/tools/lvmcmdline.c
@@ -1283,7 +1283,7 @@ struct cmd_context *init_lvm(void)
 {
 	struct cmd_context *cmd;
 
-	if (!(cmd = create_toolcontext(0, NULL)))
+	if (!(cmd = create_toolcontext(0, NULL, 1)))
 		return_NULL;
 
 	_cmdline.arg_props = &_arg_props[0];
-- 
1.7.4.4

[PATCH 5/5] Set our own in/out buffers

[Milan Broz]

On 04/19/2011 02:55 PM, Zdenek Kabelac wrote:
> Memory lock from critical_section is now being kept over the critical
> section - mallopt() should ensure, that mmap is not used for allocation,
> and we preallocate some memory to be able to satisfy some small
> alloc request. However when glibc needs buffers for line buffering of
> input and output buffers - it allocates these buffers in such way it
> adds memory page for each such buffer and size of unlock memory check will
> mismatch by 1 or 2 pages.

So this will fix that famous 1 page difference in memory lock/unlock.

> +	/* Set in/out stream buffering before glibc */
> +	if (set_buffering) {
> +		/* Allocate 2 buffers */
> +		if (!(cmd->linebuffer = dm_malloc(2 * linebuffer_size))) {
> +			log_error("Failed to allocate line buffer.");
> +			goto out;
> +		}
> +		if ((setvbuf(stdin, cmd->linebuffer, _IOLBF, linebuffer_size) ||
> +		     setvbuf(stdout, cmd->linebuffer + linebuffer_size,

IMHO I would better allocate two separate buffers here, if it uses some
stack markers to detect overflow, it will be effectively switched off
for stdin buffer.

(But it is internal glibc thing.)

> +	if (cmd->linebuffer) {
> +		/* Reset stream buffering to defaults */
> +		setlinebuf(stdin);

btw this requires _BSD_SOURCE, isn't better use
setvbuf(stream, (char *) NULL, _IOLBF, 0); ? (see man page)

> +		fflush(stdout);

(I guess flush is implicit but it is not documented so better to be safe.)


ACK, I think we should squeeze it in current release ;-)

Milan