Misc code updates
cover Misc code updates
Small patchset contains various bits of code I keep in front of my tree.
First strict-aliasing is a bit contraversional and needs bigger review.
Configure patch basicaly cleanups help strings with the use of AC_HELP_STRING.
Nextbit patch is tiny code optimization.
Backtrace patch skips printing of backtrace in the log if the situation is not an error case.
Memlock slightly modifies memory map printing.
Last patch for dmsetup returns '0' exist status when showing option help.
Zdenek Kabelac (6):
Fix compiler warning about strict-aliasing break configure changes - AC_HELP_STRING, AH_TEMPLATE nextbit
Do not show backtrace
Better alligned debug message - memlock Exit status 0 for 'dmsetup -c -o help'
configure | 100 +++++++++++++++++++++------------------------ configure.in | 76 +++++++++++++++++++--------------- lib/activate/activate.c | 6 ++- lib/device/device.c | 18 ++++---- lib/mm/memlock.c | 7 ++- libdm/datastruct/bitset.c | 4 +- tools/dmsetup.c | 1 + 7 files changed, 109 insertions(+), 103 deletions(-)
PATCH 1/6 Fix compiler warning about strict-aliasing break
This patch not really tested - and just show a possible solution for strict-alliasing error.
So patch is just a proposal of one of many solution.
--- lib/device/device.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)
lib/device/device.c +9 -9
@@ -58,9 +58,11 @@ static int _has_partition_table(struct device *dev) {int ret =0 ;unsigned p; -uint16_t buf[SECTOR_SIZE /sizeof (uint16_t )]; -uint16_t *part_magic; -struct partition *part; +struct { +uint8_t skip[PART_OFFSET ]; +struct partition part[4 ]; +uint16_t magic; + } __attribute__((packed)) buf;/* sizeof() == SECTOR_SIZE */ if (!dev_read (dev,UINT64_C (0 ),sizeof (buf), &buf)) return_0;
@@ -68,17 +70,15 @@ static int _has_partition_table(struct device *dev)/* FIXME Check for other types of partition table too */ /* Check for msdos partition table */ - part_magic = buf +PART_MAGIC_OFFSET /sizeof (buf[0 ]); -if ((*part_magic ==xlate16 (PART_MAGIC ))) { - part = (struct partition *) (buf +PART_OFFSET /sizeof (buf[0 ])); -for (p =0 ; p <4 ; p++, part++) { +if (buf.magic ==xlate16 (PART_MAGIC )) { +for (p =0 ; p <4 ; ++p) {/* Table is invalid if boot indicator not 0 or 0x80 */ -if ((part->boot_ind &0x7f )) { +if (buf.part[p].boot_ind &0x7f ) { ret =0 ;break ; }/* Must have at least one non-empty partition */ -if (part->nr_sects) +if (buf.part[p].nr_sects) ret =1 ; } }
Signature · 1 line
1.7.1.1
PATCH 1/6 Fix compiler warning about strict-aliasing break
Quoted 4 lines · Zdenek Kabelac
This patch not really tested - and just show a possible solution for strict-alliasing error.
So patch is just a proposal of one of many solution.
The patch looks OK, the code is actually cleaner that way. You can also drop PART_MAGIC_OFFSET which is no longer used (I have checked that it is indeed PART_OFFSET + 4*sizeof(struct partition)).
I believe the code is equivalent to what was there before. Nevertheless, a regression test that would cover this area would be more than welcome.
Yours,
Petr.
PATCH 2/6 configure changes - AC_HELP_STRING, AH_TEMPLATE
In reply to Zdenek Kabelacchange don't -> do not
--- configure | 100 +++++++++++++++++++++++++++------------------------------ configure.in | 76 ++++++++++++++++++++++++-------------------- 2 files changed, 89 insertions(+), 87 deletions(-)
configure +47 -53
@@ -1535,7 +1535,7 @@ Optional Features: --enable-udev_rules Install rule files needed for udev synchronisation --enable-compat Enable support for old device-mapper versions --enable-units-compat Enable output compatibility with old versions that - that don't use KiB-style unit suffixes + that do not use KiB-style unit suffixes --disable-driver Disable calls to device-mapper in the kernel --disable-o_direct Disable O_DIRECT --enable-applib Build application library
@@ -1575,7 +1575,8 @@ Optional Packages: * all (autodetect) * none (disable build) TYPE=none - --with-cmirrord-pidfile=PATH cmirrord pidfile [/var/run/cmirrord.pid] + --with-cmirrord-pidfile=PATH + cmirrord pidfile [/var/run/cmirrord.pid] --with-optimisation=OPT C optimisation flag [OPT=-O2] --with-localedir=DIR Translation files in DIR [PREFIX/share/locale] --with-confdir=DIR Configuration files in DIR [/etc]
@@ -1584,13 +1585,20 @@ Optional Packages: --with-usrsbindir=DIR --with-udev-prefix=UPREFIX Install udev rule files in UPREFIX [EPREFIX] --with-udevdir=DIR udev rules in DIR [UPREFIX/lib/udev/rules.d] - --with-dmeventd-pidfile=PATH dmeventd pidfile [/var/run/dmeventd.pid] - --with-dmeventd-path=PATH dmeventd path [EPREFIX/sbin/dmeventd] - --with-default-system-dir=DIR Default LVM system directory [/etc/lvm] - --with-default-archive-subdir=SUBDIR Default metadata archive subdir [archive] - --with-default-backup-subdir=SUBDIR Default metadata backup subdir [backup] - --with-default-cache-subdir=SUBDIR Default metadata cache subdir [cache] - --with-default-locking-dir=DIR Default locking directory [/var/lock/lvm] + --with-dmeventd-pidfile=PATH + dmeventd pidfile [/var/run/dmeventd.pid] + --with-dmeventd-path=PATH + dmeventd path [EPREFIX/sbin/dmeventd] + --with-default-system-dir=DIR + Default LVM system directory [/etc/lvm] + --with-default-archive-subdir=SUBDIR + Default metadata archive subdir [archive] + --with-default-backup-subdir=SUBDIR + Default metadata backup subdir [backup] + --with-default-cache-subdir=SUBDIR + Default metadata cache subdir [cache] + --with-default-locking-dir=DIR + Default locking directory [/var/lock/lvm] --with-interface=IFACE Choose kernel interface (ioctl) [ioctl] Some influential environment variables:
@@ -13836,23 +13844,20 @@ $as_echo "$CMIRRORD" >&6; } BUILD_CMIRRORD=$CMIRRORD ################################################################################ - - if test "x$BUILD_CMIRRORD" = xyes; then # Check whether --with-cmirrord-pidfile was given. if test "${with_cmirrord_pidfile+set}" = set; then - withval=$with_cmirrord_pidfile; cat >>confdefs.h <<_ACEOF -#define CMIRRORD_PIDFILE "$withval" -_ACEOF - + withval=$with_cmirrord_pidfile; CMIRRORD_PIDFILE=$withval else - cat >>confdefs.h <<_ACEOF -#define CMIRRORD_PIDFILE "/var/run/cmirrord.pid" -_ACEOF - + CMIRRORD_PIDFILE="/var/run/cmirrord.pid" fi + +cat >>confdefs.h <<_ACEOF +#define CMIRRORD_PIDFILE "$CMIRRORD_PIDFILE" +_ACEOF + fi ################################################################################
@@ -17651,115 +17656,104 @@ _ACEOF fi ################################################################################ - - if test "$BUILD_DMEVENTD" = yes; then # Check whether --with-dmeventd-pidfile was given. if test "${with_dmeventd_pidfile+set}" = set; then - withval=$with_dmeventd_pidfile; cat >>confdefs.h <<_ACEOF -#define DMEVENTD_PIDFILE "$withval" -_ACEOF - + withval=$with_dmeventd_pidfile; DMEVENTD_PIDFILE=$withval else - cat >>confdefs.h <<_ACEOF -#define DMEVENTD_PIDFILE "/var/run/dmeventd.pid" -_ACEOF - + DMEVENTD_PIDFILE="/var/run/dmeventd.pid" fi -fi +cat >>confdefs.h <<_ACEOF +#define DMEVENTD_PIDFILE "$DMEVENTD_PIDFILE" +_ACEOF +fi if test "$BUILD_DMEVENTD" = yes; then # Check whether --with-dmeventd-path was given. if test "${with_dmeventd_path+set}" = set; then - withval=$with_dmeventd_path; cat >>confdefs.h <<_ACEOF -#define DMEVENTD_PATH "$withval" -_ACEOF - + withval=$with_dmeventd_path; DMEVENTD_PATH=$withval else - cat >>confdefs.h <<_ACEOF -#define DMEVENTD_PATH "$lvm_exec_prefix/sbin/dmeventd" -_ACEOF - + DMEVENTD_PATH="$lvm_exec_prefix/sbin/dmeventd" fi -fi -################################################################################ +cat >>confdefs.h <<_ACEOF +#define DMEVENTD_PATH "$DMEVENTD_PATH" +_ACEOF +fi +################################################################################ # Check whether --with-default-system-dir was given. if test "${with_default_system_dir+set}" = set; then - withval=$with_default_system_dir; DEFAULT_SYS_DIR="$withval" + withval=$with_default_system_dir; DEFAULT_SYS_DIR=$withval else DEFAULT_SYS_DIR="/etc/lvm" fi + cat >>confdefs.h <<_ACEOF #define DEFAULT_SYS_DIR "$DEFAULT_SYS_DIR" _ACEOF - - # Check whether --with-default-archive-subdir was given. if test "${with_default_archive_subdir+set}" = set; then - withval=$with_default_archive_subdir; DEFAULT_ARCHIVE_SUBDIR="$withval" + withval=$with_default_archive_subdir; DEFAULT_ARCHIVE_SUBDIR=$withval else DEFAULT_ARCHIVE_SUBDIR="archive" fi + cat >>confdefs.h <<_ACEOF #define DEFAULT_ARCHIVE_SUBDIR "$DEFAULT_ARCHIVE_SUBDIR" _ACEOF - - # Check whether --with-default-backup-subdir was given. if test "${with_default_backup_subdir+set}" = set; then - withval=$with_default_backup_subdir; DEFAULT_BACKUP_SUBDIR="$withval" + withval=$with_default_backup_subdir; DEFAULT_BACKUP_SUBDIR=$withval else DEFAULT_BACKUP_SUBDIR="backup" fi + cat >>confdefs.h <<_ACEOF #define DEFAULT_BACKUP_SUBDIR "$DEFAULT_BACKUP_SUBDIR" _ACEOF - - # Check whether --with-default-cache-subdir was given. if test "${with_default_cache_subdir+set}" = set; then - withval=$with_default_cache_subdir; DEFAULT_CACHE_SUBDIR="$withval" + withval=$with_default_cache_subdir; DEFAULT_CACHE_SUBDIR=$withval else DEFAULT_CACHE_SUBDIR="cache" fi + cat >>confdefs.h <<_ACEOF #define DEFAULT_CACHE_SUBDIR "$DEFAULT_CACHE_SUBDIR" _ACEOF - - # Check whether --with-default-locking-dir was given. if test "${with_default_locking_dir+set}" = set; then - withval=$with_default_locking_dir; DEFAULT_LOCK_DIR="$withval" + withval=$with_default_locking_dir; DEFAULT_LOCK_DIR=$withval else DEFAULT_LOCK_DIR="/var/lock/lvm" fi + cat >>confdefs.h <<_ACEOF #define DEFAULT_LOCK_DIR "$DEFAULT_LOCK_DIR" _ACEOF
configure.in +42 -34
120 lines not on this page · raw message
Signature · 1 line
1.7.1.1
PATCH 2/6 configure changes - AC_HELP_STRING, AH_TEMPLATE
Whatever.
Ack.
Alasdair
PATCH 3/6 nextbit
In reply to Zdenek Kabelac--- libdm/datastruct/bitset.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
libdm/datastruct/bitset.c +2 -2
@@ -69,9 +69,9 @@ void dm_bit_union(dm_bitset_t out, dm_bitset_t in1, dm_bitset_t in2)static int _test_word (uint32_t test,int bit) { -int next_set_bit; +uint32_t tb = test >> bit; -return ((next_set_bit =ffs (test >> bit)) ? next_set_bit + bit -1 :-1 ); +return (tb ?ffs (tb) + bit -1 :-1 ); }int dm_bit_get_next (dm_bitset_t bs,int last_bit)
Signature · 1 line
1.7.1.1
PATCH 3/6 nextbit
Ack
Alasdair
PATCH 4/6 Do not show backtrace
In reply to Zdenek KabelacWhen we expect this state and behavior is correct.
FIXME: Add resonable error message instead of 'goto_out'
--- lib/activate/activate.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
lib/activate/activate.c +4 -2
@@ -961,8 +961,10 @@ static int _lv_resume(struct cmd_context *cmd, const char *lvid_s, goto_out;if (!info.exists || !info.suspended) { - r = error_if_not_active ?0 :1 ; - goto_out; +if (error_if_not_active) + goto_out; + r =1 ; + goto out; }if (!_lv_activate_lv (lv))
Signature · 1 line
1.7.1.1
PATCH 4/6 Do not show backtrace
Ack.
Alasdair
PATCH 5/6 Better alligned debug message - memlock
In reply to Zdenek Kabelac--- lib/mm/memlock.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
lib/mm/memlock.c +4 -3
@@ -137,7 +137,8 @@ static int _maps_line(struct cmd_context *cmd, lvmlock_t lock,/* Select readable maps */ if (fr !='r' ) { -log_debug ("mlock area unreadable '%s': Skipping." , line); +log_debug ("m%slock area unreadable %s : Skipping." , + (lock ==LVM_MLOCK ) ?"" :"un" , line);return 1 ; }
@@ -171,8 +172,8 @@ static int _maps_line(struct cmd_context *cmd, lvmlock_t lock, } *mstats += sz; -log_debug ("%s %10ldKiB %12lx - %12lx %c%c%c%c %s" , - (lock ==LVM_MLOCK ) ?"mlock" :"munlock" , +log_debug ("m%slock %10ldKiB %12lx - %12lx %c%c%c%c%s" , + (lock ==LVM_MLOCK ) ?"" :"un" , ((long )sz +1023 ) /1024 , from, to, fr, fw, fx, fp, line + pos);if (lock ==LVM_MLOCK ) {
Signature · 1 line
1.7.1.1
PATCH 5/6 Better alligned debug message - memlock
Ack.
lib/mm/memlock.c static int _maps_line(struct cmd_context *cmd, lvmlock_t lock, · line 176
Quoted 4 lines · Zdenek Kabelac
lib/mm/memlock.c +2
-2- log_debug("%s %10ldKiB %12lx - %12lx %c%c%c%c %s", - (lock == LVM_MLOCK) ? "mlock" : "munlock", + log_debug("m%slock %10ldKiB %12lx - %12lx %c%c%c%c%s", + (lock == LVM_MLOCK) ? "" : "un",
?!
Alasdair
PATCH 6/6 Exit status 0 for 'dmsetup -c -o help'
In reply to Zdenek Kabelac--- tools/dmsetup.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
tools/dmsetup.c +1 -0
@@ -3340,6 +3340,7 @@ int main(int argc, char **argv) }if (argc ==0 ) { + r =0 ;_usage (stderr); goto out; }
Signature · 1 line
1.7.1.1
PATCH 6/6 Exit status 0 for 'dmsetup -c -o help'
tools/dmsetup.c int main(int argc, char **argv) · line 3345
Quoted 11 lines · Zdenek Kabelac
tools/dmsetup.c +1
-0@@ -3340,6 +3340,7 @@ int main(int argc, char **argv) }if (argc ==0 ) { + r =0 ;_usage (stderr); goto out;
How does that work?
# dmsetup
Should give failure status.
(It already does.)
# dmsetup help
Should give success.
(It already does.)
# dmsetup -c -o help
Should give success.
(It doesn't.)
Alasdair
PATCH 6/6 Exit status 0 for 'dmsetup -c -o help'
Dne 7.7.2010 20:16, Alasdair G Kergon napsal(a):
Quoted 22 lines · Zdenek Kabelac
tools/dmsetup.c int main(int argc, char **argv) · line 3345
Quoted 11 lines
tools/dmsetup.c +1
-0@@ -3340,6 +3340,7 @@ int main(int argc, char **argv) }if (argc ==0 ) { + r =0 ;_usage (stderr); goto out;How does that work?
# dmsetupShould give failure status.
(It already does.)# dmsetup help
Quoted 8 lines
Should give success.
(It already does.)# dmsetup -c -o helpShould give success.
(It doesn't.)
Here is second try to address only this explicit problem:
_report_init() is able to return 1 for 'help'/'?'
(same check is in libdm-report.c)
Following checks detects that 'info' invoked it.
What still looks weird is 'dmsetup ls -c -o help' - which returns 1 - but displays meaningless help message for this command, (but this is not influenced by this patch).
Zdenek
Index: tools/dmsetup.c
===================================================================
RCS file: /cvs/lvm2/LVM2/tools/dmsetup.c,v
retrieving revision 1.141
dmsetup.c +12 -2
@@ -2652,6 +2652,9 @@ static int _report_init(struct command * r =1 ; out: +if (!strcasecmp (options,"help" ) || !strcmp (options,"?" )) + r =1 ; +if (len)dm_free (options);
@@ -3360,8 +3363,15 @@ int main(int argc, char **argv)if (!_switches[COLS_ARG ] && !strcmp (c->name,"splitname" )) _switches[COLS_ARG ]++; -if (_switches[COLS_ARG ] && !_report_init (c)) - goto out; +if (_switches[COLS_ARG ]) { +if (!_report_init (c)) + goto out; +if (!_report) { +if (!strcmp (c->name,"info" )) + r =0 ;/* info -c -o help */ + goto out; + } + }#ifdef UDEV_SYNC_SUPPORT if (!_set_up_udev_support (dev_dir))