Fixes
cover Fixes
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(-)
Signature · 1 line
1.7.4.4
PATCH 1/5 change API to use unsigned for #copies
--- lib/metadata/metadata-exported.h | 2 +- lib/metadata/metadata.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
lib/metadata/metadata-exported.h +1 -1
@@ -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,
lib/metadata/metadata.c +1 -1
@@ -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) {
Signature · 1 line
1.7.4.4
PATCH 2/5 Cast to 'int'
In reply to Zdenek KabelacAs we use both - unsigned and signed ints to decode major number - cast it to 'int' to avoid warnings here.
--- lib/filters/filter.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
lib/filters/filter.h +1 -1
@@ -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
Signature · 1 line
1.7.4.4
PATCH 2/5 Cast to 'int'
lib/filters/filter.h line 27
Quoted 6 lines · Zdenek Kabelac
#ifdef linux
lib/filters/filter.h +1
-1-# 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
In reply to Zdenek KabelacFunny 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)
#endifTo keep warning quiet - cast to the 'expected' int.
--- .../dmeventd/plugins/snapshot/dmeventd_snapshot.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
daemons/dmeventd/plugins/snapshot/dmeventd_snapshot.c +2 -2
@@ -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.",
Signature · 1 line
1.7.4.4
PATCH 4/5 Ignore dm_snprintf result
In reply to Zdenek Kabelac--- lib/log/log.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
lib/log/log.c +2 -2
@@ -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' ;
Signature · 1 line
1.7.4.4
PATCH 5/5 Set our own in/out buffers
In reply to Zdenek KabelacMemory 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.
--- 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(-)
daemons/clvmd/lvm-functions.c +1 -1
@@ -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 ; }
lib/commands/toolcontext.c +29 -1
@@ -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 ();
lib/commands/toolcontext.h +3 -1
@@ -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);
liblvm/lvm_base.c +1 -1
@@ -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 ;
tools/lvmcmdline.c +1 -1
@@ -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 ];
Signature · 1 line
1.7.4.4
PATCH 5/5 Set our own in/out buffers
Quoted 7 lines · Zdenek Kabelac
Memory lock from
critical_sectionis 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.
lib/commands/toolcontext.c struct cmd_context *create_toolcontext(unsigned is_long_lived, · line 1176
Quoted 9 lines
lib/commands/toolcontext.c +9
-0+/* 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.)
lib/commands/toolcontext.c void destroy_toolcontext(struct cmd_context *cmd) · line 1423
Quoted 3 lines
lib/commands/toolcontext.c +3
-0+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)
lib/commands/toolcontext.c void destroy_toolcontext(struct cmd_context *cmd) · line 1424
Quoted 1 line
lib/commands/toolcontext.c +1
-0+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