[PATCH 1/3] use display_size function instead of hardcoding

[PATCH 1/3] use display_size function instead of hardcoding

[Daniel Mierswa]

We should use display_size everywhere for consistency reasons. Other
commands and places that print the stripe/PE size already do that.
This might break scripts that rely on the stripe/PE size being printed
in KByte in those circumstances.
This requires changes in the .po file aswell.

Signed-off-by: Daniel Mierswa <impulze@impulze.org>
---
 lib/display/display.c |    4 ++--
 lib/striped/striped.c |    4 ++--
 po/lvm2.po            |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/display/display.c b/lib/display/display.c
index 62f30f6..b8da95b 100644
--- a/lib/display/display.c
+++ b/lib/display/display.c
@@ -344,7 +344,7 @@ void pvdisplay_full(const struct cmd_context *cmd,
 	/* LV count is no longer available when displaying PV
 	   log_print("Cur LV                %u", vg->lv_count);
 	 */
-	log_print("PE Size (KByte)       %" PRIu32, pv->pe_size / 2);
+	log_print("PE Size               %s", display_size(cmd, (uint64_t) pv->pe_size));
 	log_print("Total PE              %u", pv->pe_count);
 	log_print("Free PE               %" PRIu32, pe_free);
 	log_print("Allocated PE          %u", pv->pe_alloc_count);
@@ -489,7 +489,7 @@ int lvdisplay_full(struct cmd_context *cmd,
 	log_print("Segments               %u", dm_list_size(&lv->segments));
 
 /********* FIXME Stripes & stripesize for each segment
-	log_print("Stripe size (KByte)    %u", lv->stripesize / 2);
+	log_print("Stripe size            %s", display_size(cmd, (uint64_t) lv->stripesize));
 ***********/
 
 	log_print("Allocation             %s", get_alloc_string(lv->alloc));
diff --git a/lib/striped/striped.c b/lib/striped/striped.c
index 0e1ca2d..072e34b 100644
--- a/lib/striped/striped.c
+++ b/lib/striped/striped.c
@@ -40,8 +40,8 @@ static void _striped_display(const struct lv_segment *seg)
 		display_stripe(seg, 0, "  ");
 	else {
 		log_print("  Stripes\t\t%u", seg->area_count);
-		log_print("  Stripe size\t\t%u KB", seg->stripe_size / 2);
-
+		log_print("  Stripe size\t\t%s", display_size(seg->lv->vg->cmd,
+		                                              (uint64_t) seg->stripe_size));
 		for (s = 0; s < seg->area_count; s++) {
 			log_print("  Stripe %d:", s);
 			display_stripe(seg, s, "    ");
diff --git a/po/lvm2.po b/po/lvm2.po
index 2b236ae..8a14d7b 100644
--- a/po/lvm2.po
+++ b/po/lvm2.po
@@ -1487,7 +1487,7 @@ msgstr ""
 
 #: display/display.c:312
 #, c-format
-msgid "PE Size (KByte)       %u"
+msgid "PE Size               %s"
 msgstr ""
 
 #: display/display.c:313 display/display.c:592
@@ -6510,7 +6510,7 @@ msgstr ""
 
 #: striped/striped.c:42
 #, c-format
-msgid "  Stripe size\t\t%u KB"
+msgid "  Stripe size\t\t%s"
 msgstr ""
 
 #: striped/striped.c:45
-- 
1.6.4.4

[PATCH 2/3] differentiate between S.I. and binary units in output

[Daniel Mierswa]

Current behaviour would print identical strings for S.I. and binary
based units, say KB or Megabyte, also K/M/G in short notation.
This can be confusing at times, because the manual for lvs/pvs/vgs etc.
all state that you pass lowercase suffixes for binary and uppercase
suffixes for S.I. suffixes.
This patch attempts to differentiate between those units in the output
and print a proper string accordingly.
For binary based uints:
 Long output strings (unsed) will be Kibibyte, Mebibyte, etc.
 Short output strings will be KiB, MiB, etc.
 Unit output strings will be lower-cased: k, m, g, etc.
 Sectors determined by division with 512 will be suffixed with s.
For S.I. based units:
 Long output strings (unused) will be Kilobyte, Megabyte, etc.
 Short output strings will be kB, MB, GB, etc.
 Unit output strings will be upper-cased: K, M, G, etc.
 Sectors determined by division with 500 will be suffixed with S.

This will break every script that relies on the output of commands,
which would print "KB", "MB", etc.
They need to be adjusted to respect above documented notions and also
take into consideration that padding changed, i.e. " MB" = " MB ", since
" MiB" and so forth have 3 letters. See the patch for details.

Signed-off-by: Daniel Mierswa <impulze@impulze.org>
---
 lib/display/display.c |   54 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/lib/display/display.c b/lib/display/display.c
index b8da95b..1be7366 100644
--- a/lib/display/display.c
+++ b/lib/display/display.c
@@ -156,20 +156,30 @@ static const char *_display_size(const struct cmd_context *cmd,
 	int s;
 	int suffix = 1, precision;
 	uint64_t byte = UINT64_C(0);
-	uint64_t units = UINT64_C(1024);
 	char *size_buf = NULL;
 	const char * const size_str[][3] = {
-		{" Exabyte", " EB", "E"},
-		{" Petabyte", " PB", "P"},
-		{" Terabyte", " TB", "T"},
-		{" Gigabyte", " GB", "G"},
-		{" Megabyte", " MB", "M"},
-		{" Kilobyte", " KB", "K"},
+		{" Exbibyte", " EiB", "e"},
+		{" Pebibyte", " PiB", "p"},
+		{" Tebibyte", " TiB", "t"},
+		{" Gibibyte", " GiB", "g"},
+		{" Mebibyte", " MiB", "m"},
+		{" Kibibyte", " KiB", "k"},
+		{" Sectors ", " Se ", "s"},
+
+		{" Exabyte ", " EB ", "E"},
+		{" Petabyte", " PB ", "P"},
+		{" Terabyte", " TB ", "T"},
+		{" Gigabyte", " GB ", "G"},
+		{" Megabyte", " MB ", "M"},
+		{" Kilobyte", " kB ", "K"},
+		{" Sectors ", " Se ", "S"},
+
 		{"", "", ""},
-		{" Byte    ", " B ", "B"},
-		{" Units   ", " Un", "U"},
-		{" Sectors ", " Se", "S"},
-		{"         ", "   ", " "},
+
+		{" Byte    ", " B  ", "B"},
+		{" Units   ", " Un ", "U"},
+
+		{"         ", "    ", " "},
 	};
 
 	if (!(size_buf = dm_pool_alloc(cmd->mem, SIZE_BUF))) {
@@ -179,9 +189,14 @@ static const char *_display_size(const struct cmd_context *cmd,
 
 	suffix = cmd->current_settings.suffix;
 
-	for (s = 0; s < 10; s++)
-		if (toupper((int) cmd->current_settings.unit_type) ==
-		    *size_str[s][2])
+	/* find one of kmgtpesKMGTPES */
+	for (s = 0; s < 14; s++)
+		if (cmd->current_settings.unit_type == *size_str[s][2])
+			break;
+
+	/* find one of bBuU */
+	for (; s >= 14 && s < 17; s++)
+		if (toupper((int) cmd->current_settings.unit_type) == *size_str[s][2])
 			break;
 
 	if (size == UINT64_C(0)) {
@@ -191,16 +206,19 @@ static const char *_display_size(const struct cmd_context *cmd,
 
 	size *= UINT64_C(512);
 
-	if (s < 10)
+	if (s < 17)
 		byte = cmd->current_settings.unit_factor;
 	else {
+		uint64_t units;
 		suffix = 1;
-		if (cmd->current_settings.unit_type == 'H')
+		if (cmd->current_settings.unit_type == 'H') {
+			s = 6;
 			units = UINT64_C(1000);
-		else
+		} else {
+			s = 0;
 			units = UINT64_C(1024);
+		}
 		byte = units * units * units * units * units * units;
-		s = 0;
 		while (size_str[s] && size < byte)
 			s++, byte /= units;
 	}
-- 
1.6.4.4

[PATCH 2/3] differentiate between S.I. and binary units in output

[Alasdair G Kergon]

On Sat, Sep 26, 2009 at 05:33:46PM +0200, Daniel Mierswa wrote:
> For binary based uints:
>  Long output strings (unsed) will be Kibibyte, Mebibyte, etc.
>  Short output strings will be KiB, MiB, etc.
>  Unit output strings will be lower-cased: k, m, g, etc.
>  Sectors determined by division with 512 will be suffixed with s.
> For S.I. based units:
>  Long output strings (unused) will be Kilobyte, Megabyte, etc.
>  Short output strings will be kB, MB, GB, etc.
>  Unit output strings will be upper-cased: K, M, G, etc.
>  Sectors determined by division with 500 will be suffixed with S.

Bytes, Units and Sectors are special cases - in a separate group.
A sector is always 512 bytes.  I don't see any need to support 500-byte
sectors.

As with the last patch, I can't put this into RHEL5 so it needs to
be conditional (selectable at both compilation time and run time).

Alasdair.

Re: [PATCH 2/3] differentiate between S.I. and binary units in output

[Mike Snitzer]

On Mon, Sep 28 2009 at  8:49am -0400,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Sat, Sep 26, 2009 at 05:33:46PM +0200, Daniel Mierswa wrote:
> > For binary based uints:
> >  Long output strings (unsed) will be Kibibyte, Mebibyte, etc.
> >  Short output strings will be KiB, MiB, etc.
> >  Unit output strings will be lower-cased: k, m, g, etc.
> >  Sectors determined by division with 512 will be suffixed with s.
> > For S.I. based units:
> >  Long output strings (unused) will be Kilobyte, Megabyte, etc.
> >  Short output strings will be kB, MB, GB, etc.
> >  Unit output strings will be upper-cased: K, M, G, etc.
> >  Sectors determined by division with 500 will be suffixed with S.
> 
> Bytes, Units and Sectors are special cases - in a separate group.
> A sector is always 512 bytes.  I don't see any need to support 500-byte
> sectors.

I agree that 500-byte sectors aren't meaningful but I added support for
them a few months ago to be consistent, see:
http://sources.redhat.com/git/?p=lvm2.git;a=commit;h=69da2ac0

Re: [PATCH 2/3] differentiate between S.I. and binary units in output

[Alasdair G Kergon]

On Mon, Sep 28, 2009 at 09:17:59AM -0400, Mike Snitzer wrote:
> I agree that 500-byte sectors aren't meaningful but I added support for
> them a few months ago to be consistent, see:
> http://sources.redhat.com/git/?p=lvm2.git;a=commit;h=69da2ac0

Where's that in WHATS_NEW?

I'll probably take it back out: I can't think of any reason for supporting it
and confusing people.

Alasdair

Re: [PATCH 2/3] differentiate between S.I. and binary units in output

[Mike Snitzer]

On Mon, Sep 28 2009 at 10:25am -0400,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Mon, Sep 28, 2009 at 09:17:59AM -0400, Mike Snitzer wrote:
> > I agree that 500-byte sectors aren't meaningful but I added support for
> > them a few months ago to be consistent, see:
> > http://sources.redhat.com/git/?p=lvm2.git;a=commit;h=69da2ac0
> 
> Where's that in WHATS_NEW?
> 
> I'll probably take it back out: I can't think of any reason for supporting it
> and confusing people.

Right, I agree that confusing people isn't good.. but I thought it
better to allow people to specify 'S' without erroring out.

As for WHATS_NEW; I added the following to the 2.02.49 release:
Update the man pages to document size units uniformly.
Allow commandline sizes to be specified in terms of bytes and sectors.

If you do back it out please still allow 's'; it is needed for pvcreate
--dataalignmentoffset 

But as we discussed, its probably best if both 's' and 'S' mean 512
bytes.

Mike

[PATCH 2/3] differentiate between S.I. and binary units in output

[Alasdair G Kergon]

So please would you:
  1) Make this change conditional on a runtime check of
cmd->si_unit_consistency;
  2) Remove the 500-byte sector offering.

The new-style output can be the default from 2.02.54 onwards - Fedora
13, RHEL6 etc.

People who find problems with their scripts after an upgrade can easily
reinstate the old inconsistent output by using an lvm.conf setting.

Distros that can't change the output (e.g. RHEL5) can set a configure
parameter.

Alasdair

[PATCH 2/3] differentiate between S.I. and binary units in output

[Daniel Mierswa]

On 28.09.2009 18:44, Alasdair G Kergon wrote:
> So please would you:
>    1) Make this change conditional on a runtime check of
> cmd->si_unit_consistency;
Attached, hope it's bug free, somehow all things went crazy and I got 
very busy this week but don't couldn't wait any longer to get this merged.

>    2) Remove the 500-byte sector offering.
Figured, you already did.

> The new-style output can be the default from 2.02.54 onwards - Fedora
> 13, RHEL6 etc.
We still need to tweak the documentation at places, which I can do as 
soon as the patches are for the actual code handling are applied.

> People who find problems with their scripts after an upgrade can easily
> reinstate the old inconsistent output by using an lvm.conf setting.
Yep, win win. Good solution in the end anyway although it requires the 
LVM2 development team to keep "old" code around, which I prefer not to 
if I can avoid it. ;)

-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply 
know better than you do.
                --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-differentiate-between-S.I.-and-binary-units-in-outpu.patch
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090930/394a27a0/attachment.ksh>

[PATCH 2/3] differentiate between S.I. and binary units in output

[Alasdair G Kergon]

OK - I've applied this.

I added a 'base' value within the array and some #defines to try to make
the code easier to understand.  Hopefully I didn't introduce any bugs
during that process.

Alasdair

[PATCH 3/3] adjust the testsuite to work with new output

[Daniel Mierswa]

The last change made LVM tools print different size strings and the
testsuite needs to be adjusted to recognize those.

Signed-off-by: Daniel Mierswa <impulze@impulze.org>
---
 test/t-lvcreate-usage.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/t-lvcreate-usage.sh b/test/t-lvcreate-usage.sh
index 741351d..4180a96 100755
--- a/test/t-lvcreate-usage.sh
+++ b/test/t-lvcreate-usage.sh
@@ -30,7 +30,7 @@ grep "^  Negative stripesize is invalid\$" err
 
 # 'lvcreate rejects a too-large stripesize'
 not lvcreate -L 64M -n $lv -i2 --stripesize 4294967291 $vg 2>err
-grep "^  Stripe size cannot be larger than 512.00 GB\$" err
+grep "^  Stripe size cannot be larger than 512.00 GiB\$" err
 
 # 'lvcreate w/single stripe succeeds with diagnostics to stdout' 
 lvcreate -L 64M -n $lv -i1 --stripesize 4 $vg >out 2>err 
@@ -53,7 +53,7 @@ grep "^  Number of stripes (129) must be between 1 and 128\$" err
 # The case on lvdisplay output is to verify that the LV was not created.
 # 'lvcreate rejects an invalid stripe size'
 not lvcreate -L 64M -n $lv -i2 --stripesize 3 $vg 2>err
-grep "^  Invalid stripe size 3\.00 KB\$" err
+grep "^  Invalid stripe size 3\.00 KiB\$" err
 case $(lvdisplay $vg) in "") true ;; *) false ;; esac
 
 # Setting max_lv works. (bz490298)
-- 
1.6.4.4

[PATCH 1/3] use display_size function instead of hardcoding

[Alasdair G Kergon]

On Sat, Sep 26, 2009 at 05:33:45PM +0200, Daniel Mierswa wrote:
> We should use display_size everywhere for consistency reasons. Other
> commands and places that print the stripe/PE size already do that.
> This might break scripts that rely on the stripe/PE size being printed
> in KByte in those circumstances.

I can't make this change in RHEL5 and since we use the upstream code base
that means we need to make it conditional, probably with an lvm.conf setting
with a default value chosen at configuration time.

Alasdair

[PATCH 1/3] use display_size function instead of hardcoding

[Alasdair G Kergon]

OK.

I've applied this one, but made it conditional on a new config setting.

Alasdair