[PATCH] Change process_each_lv_in_vg() iterator to only process non-hidden LVs.

[PATCH] Change process_each_lv_in_vg() iterator to only process non-hidden LVs.

[Dave Wysochanski]

Unless the "--all" flag is given, the iterator functions should not be
processing hidden LVs.  I have checked all the tools and this seems to
be how they all behave.  Somehow before I missed the fact I could re-use
the "--all" flag to qualify the hidden LV check (perhaps it's because
the all flag is not documented well).  If there are things I've overlooked
I can deal with them in another patch.  I am working on patches to clarify
and document the usage of "--all".

Remove redundant checks in a few tools.

This addresses rhbz 232499 (benign error messages when lvchange used
with VG containing hidden LVs).

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 tools/lvdisplay.c |    3 ---
 tools/lvscan.c    |    3 ---
 tools/reporter.c  |    6 ------
 tools/toollib.c   |    3 +++
 4 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/tools/lvdisplay.c b/tools/lvdisplay.c
index f5531cb..65c1012 100644
--- a/tools/lvdisplay.c
+++ b/tools/lvdisplay.c
@@ -18,9 +18,6 @@
 static int _lvdisplay_single(struct cmd_context *cmd, struct logical_volume *lv,
 			     void *handle)
 {
-	if (!arg_count(cmd, all_ARG) && !lv_is_visible(lv))
-		return ECMD_PROCESSED;
-
 	if (arg_count(cmd, colon_ARG))
 		lvdisplay_colons(lv);
 	else {
diff --git a/tools/lvscan.c b/tools/lvscan.c
index a65bd1d..a4f419d 100644
--- a/tools/lvscan.c
+++ b/tools/lvscan.c
@@ -28,9 +28,6 @@ static int lvscan_single(struct cmd_context *cmd, struct logical_volume *lv,
 
 	const char *active_str, *snapshot_str;
 
-	if (!arg_count(cmd, all_ARG) && !lv_is_visible(lv))
-		return ECMD_PROCESSED;
-
 	inkernel = lv_info(cmd, lv, &info, 1, 0) && info.exists;
 	if (lv_is_origin(lv)) {
 		dm_list_iterate_items_gen(snap_seg, &lv->snapshot_segs,
diff --git a/tools/reporter.c b/tools/reporter.c
index 1e93689..6186d5b 100644
--- a/tools/reporter.c
+++ b/tools/reporter.c
@@ -33,9 +33,6 @@ static int _vgs_single(struct cmd_context *cmd __attribute((unused)),
 static int _lvs_single(struct cmd_context *cmd, struct logical_volume *lv,
 		       void *handle)
 {
-	if (!arg_count(cmd, all_ARG) && !lv_is_visible(lv))
-		return ECMD_PROCESSED;
-
 	if (!report_object(handle, lv->vg, lv, NULL, NULL, NULL)) {
 		stack;
 		return ECMD_FAILED;
@@ -116,9 +113,6 @@ static int _pvsegs_sub_single(struct cmd_context *cmd,
 static int _lvsegs_single(struct cmd_context *cmd, struct logical_volume *lv,
 			  void *handle)
 {
-	if (!arg_count(cmd, all_ARG) && !lv_is_visible(lv))
-		return ECMD_PROCESSED;
-
 	return process_each_segment_in_lv(cmd, lv, handle, _segs_single);
 }
 
diff --git a/tools/toollib.c b/tools/toollib.c
index d14dc77..d85f731 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -122,6 +122,9 @@ int process_each_lv_in_vg(struct cmd_context *cmd,
 		if (lvl->lv->status & SNAPSHOT)
 			continue;
 
+		if (!lv_is_visible(lvl->lv) && !arg_count(cmd, all_ARG))
+			continue;
+
 		if (lv_is_virtual_origin(lvl->lv) && !arg_count(cmd, all_ARG))
 			continue;
 
-- 
1.6.0.6

[PATCH] Change process_each_lv_in_vg() iterator to only process non-hidden LVs.

[Milan Broz]

On 10/15/2009 04:07 PM, Dave Wysochanski wrote:
> Unless the "--all" flag is given, the iterator functions should not be
> processing hidden LVs.  I have checked all the tools and this seems to
> be how they all behave.  Somehow before I missed the fact I could re-use
> the "--all" flag to qualify the hidden LV check (perhaps it's because
> the all flag is not documented well).  If there are things I've overlooked
> I can deal with them in another patch.  I am working on patches to clarify
> and document the usage of "--all".

Ack for the intention (see my previous patch) but nack to use of all_ARG :-)

We should really document --all first, missing in man pages
(lvs, lvdisplay, lvscan, pvs, pvdisplay, vgs)


> --- a/tools/toollib.c
> +++ b/tools/toollib.c
> @@ -122,6 +122,9 @@ int process_each_lv_in_vg(struct cmd_context *cmd,
>  		if (lvl->lv->status & SNAPSHOT)
>  			continue;
>  
> +		if (!lv_is_visible(lvl->lv) && !arg_count(cmd, all_ARG))
> +			continue;
> +

This function is used used in process_each_lv().

If you do this and command uses all_ARG, caller can activate hidden
volumes directly (see lvchange).

But you cannot remove hidden volumes now (see lvremove, it has no all arg).

I think that

 - not visible (hidden) volumes must be always be activated through some
visible LV or other operation
(I have special hidden keystore volume and I never want user to activate it
explicitly for example.)

 - we should allow user to remove hidden volume (if it is unreferenced),
(like hidden orphaned mirror log when something went wrong)

- also see my previous patch for vgchange, it should be cleaned similar way
(adding exceptions for every type of hidden volume is not ideal,
lv_is_visible() seems to be ok here)

Milan

[PATCH] Change process_each_lv_in_vg() iterator to only process non-hidden LVs.

[Dave Wysochanski]

On Fri, 2009-10-16 at 11:05 +0200, Milan Broz wrote:
> On 10/15/2009 04:07 PM, Dave Wysochanski wrote:
> > Unless the "--all" flag is given, the iterator functions should not be
> > processing hidden LVs.  I have checked all the tools and this seems to
> > be how they all behave.  Somehow before I missed the fact I could re-use
> > the "--all" flag to qualify the hidden LV check (perhaps it's because
> > the all flag is not documented well).  If there are things I've overlooked
> > I can deal with them in another patch.  I am working on patches to clarify
> > and document the usage of "--all".
> 
> Ack for the intention (see my previous patch) but nack to use of all_ARG :-)
> 
> We should really document --all first, missing in man pages
> (lvs, lvdisplay, lvscan, pvs, pvdisplay, vgs)
> 

Right - I'm working on that.

Basically the way --all behaves today is:
- PV: process all devices, even if they don't have a label on them
- LV: process all LVs, even hidden/internal ones
- VG: process all VGs

There are some small inconsistencies but this is basically it.


> 
> > --- a/tools/toollib.c
> > +++ b/tools/toollib.c
> > @@ -122,6 +122,9 @@ int process_each_lv_in_vg(struct cmd_context *cmd,
> >  		if (lvl->lv->status & SNAPSHOT)
> >  			continue;
> >  
> > +		if (!lv_is_visible(lvl->lv) && !arg_count(cmd, all_ARG))
> > +			continue;
> > +
> 
> This function is used used in process_each_lv().
> 
> If you do this and command uses all_ARG, caller can activate hidden
> volumes directly (see lvchange).
> 

if you try --all on lvchange it will fail earlier since --all is not
allowed and conflicts with the allocation policy.  The only commands
that allow --all I've checked (see commands.h)


> But you cannot remove hidden volumes now (see lvremove, it has no all arg).
> 
> I think that
> 
>  - not visible (hidden) volumes must be always be activated through some
> visible LV or other operation
> (I have special hidden keystore volume and I never want user to activate it
> explicitly for example.)

Agreed.  I think this patch preserves that.

>  - we should allow user to remove hidden volume (if it is unreferenced),
> (like hidden orphaned mirror log when something went wrong)
> 
Ok, this is a different story.  Why is this?  Shouldn't they be using
dmsetup if LVM failed in such a way?  It seems like the scenario(s) you
describe will only take place when there's an LVM bug, which should be
fixed.  Can you envision another recovery scenario that is not an LVM
bug?

> - also see my previous patch for vgchange, it should be cleaned similar way
> (adding exceptions for every type of hidden volume is not ideal,
> lv_is_visible() seems to be ok here)
> 

I would like to put a stake in the ground and say the iterators do not
process hidden LVs and users should not really be operating on hidden
LVs by specifying them on the commandline.  The LV abstraction starts to
break down if we do that (what does "lvcreate -m1" mean then - it
creates multiple LVs, or is it one LV, a mirror?) and I'd rather not,
unless maybe in some select recovery scenarios as you say.  Even then we
should handle this some other way IMO.

I looked at adding a flag to the iterators but I didn't think it was
worth it.  I guess I just did not see an alternative to:
1) adding a flag to the iterators.  Maybe you would prefer this?
2) not adding flag to iterators and handling hidden LVs a different way
(by using --all or some other cmdline option)

I also thought --all was not the right flag to handle this longer term,
but wanted to preserve the existing behavior of the tools as a first
step.  Honestly --all is a bit goofy if you ask me but I understand
where it is used and why.

[PATCH] Document --all option in man pages, cleanup lvdisplay/pvdisplay man page.

[Dave Wysochanski]

Option --all is only partially documented currently, so document in all
commands.  Also make lvdisplay/pvdisplay man pages consistent with help
output.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 man/lvdisplay.8.in |   18 ++++++++++++++++--
 man/lvs.8.in       |    4 ++++
 man/lvscan.8.in    |    4 ++++
 man/pvdisplay.8.in |   15 +++++++++++++--
 man/pvs.8.in       |    4 ++++
 man/vgs.8.in       |    4 ++++
 6 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/man/lvdisplay.8.in b/man/lvdisplay.8.in
index 56d33f2..03eb627 100644
--- a/man/lvdisplay.8.in
+++ b/man/lvdisplay.8.in
@@ -3,21 +3,35 @@
 lvdisplay \- display attributes of a logical volume
 .SH SYNOPSIS
 .B lvdisplay
+[\-a|\-\-all]
 [\-c|\-\-colon] [\-d|\-\-debug] [\-h|\-?|\-\-help]
 [\-\-ignorelockingfailure]
-[\-\-maps] [\-P|\-\-partial]
+[\-\-maps]
+[\-\-nosuffix]
+[\-\-units hsbkmgtHKMGT]
+[\-P|\-\-partial]
+[\-\-version]
 [\-v|\-\-verbose] LogicalVolumePath [LogicalVolumePath...]
+.br
+
+.br
+.B lvdisplay \-\-columns | \-C
+[\fB lvs\fP (8) OPTIONS]
 .SH DESCRIPTION
 lvdisplay allows you to see the attributes of a logical volume
 like size, read/write status, snapshot information etc.
 .P
 \fBlvs\fP (8) is an alternative that provides the same information 
 in the style of \fBps\fP (1).  \fBlvs\fP is recommended over
-\fBlvdisplay\fP.
+\fBlvdisplay\fP.  \fBlvdisplay --columns\fP is the equivalent
+of calling \fBlvs\fP.
 
 .SH OPTIONS
 See \fBlvm\fP for common options.
 .TP
+.I \-\-all
+Process all logical volumes, even hidden/internal ones.
+.TP
 .I \-c, \-\-colon
 Generate colon separated output for easier parsing in scripts or programs.
 N.B. \fBlvs\fP (8) provides considerably more control over the output.
diff --git a/man/lvs.8.in b/man/lvs.8.in
index ff37a99..70ff1e0 100644
--- a/man/lvs.8.in
+++ b/man/lvs.8.in
@@ -3,6 +3,7 @@
 lvs \- report information about logical volumes
 .SH SYNOPSIS
 .B lvs
+[\-a|\-\-all]
 [\-\-aligned] [\-d|\-\-debug] [\-h|\-?|\-\-help]
 [\-\-ignorelockingfailure] [\-\-nameprefixes] [\-\-noheadings] [\-\-nosuffix]
 [\-o|\-\-options [+]Field[,Field]]
@@ -18,6 +19,9 @@ lvs produces formatted output about logical volumes.
 .SH OPTIONS
 See \fBlvm\fP for common options.
 .TP
+.I \-\-all
+Process all logical volumes, even hidden/internal ones.
+.TP
 .I \-\-aligned
 Use with \-\-separator to align the output columns.
 .TP
diff --git a/man/lvscan.8.in b/man/lvscan.8.in
index cdcdc1d..ebfb77a 100644
--- a/man/lvscan.8.in
+++ b/man/lvscan.8.in
@@ -3,6 +3,7 @@
 lvscan \- scan (all disks) for logical volumes
 .SH SYNOPSIS
 .B lvscan
+.RB [ \-a | \-\-all]
 .RB [ \-b | \-\-blockdevice ]
 .RB [ \-d | \-\-debug ]
 .RB [ \-h | \-\-help ]
@@ -16,6 +17,9 @@ in the system for defined logical volumes.
 .SH OPTIONS
 See \fBlvm\fP for common options.
 .TP
+.BR \-\-all
+Process all logical volumes, even hidden/internal ones.
+.TP
 .BR \-b ", " \-\-blockdevice
 Adds the device major and minor numbers to the display
 of each logical volume.
diff --git a/man/pvdisplay.8.in b/man/pvdisplay.8.in
index 40d68ad..627b7d3 100644
--- a/man/pvdisplay.8.in
+++ b/man/pvdisplay.8.in
@@ -3,16 +3,27 @@
 pvdisplay \- display attributes of a physical volume
 .SH SYNOPSIS
 .B pvdisplay
-[\-c|\-\-colon] [\-d|\-\-debug] [\-h|\-?|\-\-help] [\-s|\-\-short]
+[\-c|\-\-colon]
+[\-d|\-\-debug] [\-h|\-?|\-\-help]
+[\-\-ignorelockingfailure]
+[\-\-maps]
+[\-s|\-\-short]
+[\-\-units hsbkmgtHKMGT]
 [\-v[v]|\-\-verbose [\-\-verbose]]
 PhysicalVolumePath [PhysicalVolumePath...]
+.br
+
+.br
+.B pvdisplay \-\-columns | \-C
+[\fB pvs\fP (8) OPTIONS]
 .SH DESCRIPTION
 pvdisplay allows you to see the attributes of one or more physical volumes
 like size, physical extent size, space used for the volume group descriptor
 area and so on.
 .P
 \fBpvs\fP (8) is an alternative that provides the same information 
-in the style of \fBps\fP (1).
+in the style of \fBps\fP (1).  \fBpvdisplay --columns\fP is the equivalent
+of calling \fBpvs\fP.
 .SH OPTIONS
 See \fBlvm\fP for common options.
 .TP
diff --git a/man/pvs.8.in b/man/pvs.8.in
index 6bd693f..b6941f9 100644
--- a/man/pvs.8.in
+++ b/man/pvs.8.in
@@ -3,6 +3,7 @@
 pvs \- report information about physical volumes
 .SH SYNOPSIS
 .B pvs
+[\-a|\-\-all]
 [\-\-aligned] [\-d|\-\-debug] [\-h|\-?|\-\-help]
 [\-\-ignorelockingfailure] [\-\-nameprefixes] [\-\-noheadings] [\-\-nosuffix]
 [\-o|\-\-options [+]Field[,Field]]
@@ -18,6 +19,9 @@ pvs produces formatted output about physical volumes.
 .SH OPTIONS
 See \fBlvm\fP for common options.
 .TP
+.I \-\-all
+Process all devices, even ones without an LVM label.
+.TP
 .I \-\-aligned
 Use with \-\-separator to align the output columns.
 .TP
diff --git a/man/vgs.8.in b/man/vgs.8.in
index b9abaa8..55f679c 100644
--- a/man/vgs.8.in
+++ b/man/vgs.8.in
@@ -3,6 +3,7 @@
 vgs \- report information about volume groups
 .SH SYNOPSIS
 .B vgs
+[\-a|\-\-all]
 [\-\-aligned] [\-d|\-\-debug] [\-h|\-?|\-\-help]
 [\-\-ignorelockingfailure] [\-\-nameprefixes] [\-\-noheadings] [\-\-nosuffix]
 [\-o|\-\-options [+]Field[,Field]]
@@ -18,6 +19,9 @@ vgs produces formatted output about volume groups.
 .SH OPTIONS
 See \fBlvm\fP for common options.
 .TP
+.I \-\-all
+Process all volume groups.  Equivalent to not specifying any volume groups.
+.TP
 .I \-\-aligned
 Use with \-\-separator to align the output columns.
 .TP
-- 
1.6.0.6

[PATCH] Document --all option in man pages, cleanup lvdisplay/pvdisplay man page.

[Alasdair G Kergon]

On Tue, Oct 20, 2009 at 11:58:13PM -0400, Dave Wysochanski wrote:
>  lvdisplay \- display attributes of a logical volume

> +.B lvdisplay \-\-columns | \-C

Still doesn't match the --help text, which shows which arguments
go with which form of the command.

>  \fBlvs\fP (8) is an alternative that provides the same information 
>  in the style of \fBps\fP (1).  \fBlvs\fP is recommended over
> -\fBlvdisplay\fP.
> +\fBlvdisplay\fP.  \fBlvdisplay --columns\fP is the equivalent
> +of calling \fBlvs\fP.

Mention that under -C in the OPTIONS section.
"Calling X is the equivalent of calling Y" ('Running' or 'Executing'?)
or better, just "X is equivalent to Y".
  
>  .B lvs
> +[\-a|\-\-all]

> +Process all logical volumes, even hidden/internal ones.

Replace 'process' with a more descriptive verb based on what
'lvs' does.

Replace 'hidden/internal' with a better description of what
is meant.  I can spot no existing references to 'internal'
in this context in the man pages, and only one to 'implemented by
creating a hidden virtual device'.

Alasdair

[PATCH] Document --all option in man pages, cleanup lvdisplay/pvdisplay man page.

[Dave Wysochanski]

On Wed, 2009-10-21 at 13:51 +0100, Alasdair G Kergon wrote:
> On Tue, Oct 20, 2009 at 11:58:13PM -0400, Dave Wysochanski wrote:
> >  lvdisplay \- display attributes of a logical volume
> 
> > +.B lvdisplay \-\-columns | \-C
> 
> Still doesn't match the --help text, which shows which arguments
> go with which form of the command.
> 
> >  \fBlvs\fP (8) is an alternative that provides the same information 
> >  in the style of \fBps\fP (1).  \fBlvs\fP is recommended over
> > -\fBlvdisplay\fP.
> > +\fBlvdisplay\fP.  \fBlvdisplay --columns\fP is the equivalent
> > +of calling \fBlvs\fP.
> 
> Mention that under -C in the OPTIONS section.
> "Calling X is the equivalent of calling Y" ('Running' or 'Executing'?)
> or better, just "X is equivalent to Y".
>   
> >  .B lvs
> > +[\-a|\-\-all]
> 
> > +Process all logical volumes, even hidden/internal ones.
> 
> Replace 'process' with a more descriptive verb based on what
> 'lvs' does.
> 
> Replace 'hidden/internal' with a better description of what
> is meant.  I can spot no existing references to 'internal'
> in this context in the man pages, and only one to 'implemented by
> creating a hidden virtual device'.
> 

Agreed this needs clarification - Milan has mentioned the same thing.
I'm not sure how to clarify though given existing constraints.

We're using "--all" in the tools to display internal "LVs" but from the
user perspective, they cannot really do operations on "hidden" LVs, and
I don't think we want them to.  The internal definition of the "LV" is
different from the user's understanding of an "LV" (a user cannot create
an internal LV, etc).

What is really going on is we're allowing users to operate on top-level
LVs, or "simple LVs" via the tools, while internal or "complex" LVs we
are not.

I think we may need to explain to users this notion of
internal/hidden/complex LVs vs simple ones.  I will make a go at
explaining this via either the lvcreate man page or lvm man page.