[PATCH 12/12] Remove init_full_scan_done(0) call after create_toolcontext().

[PATCH 12/12] Remove init_full_scan_done(0) call after create_toolcontext().

[Dave Wysochanski]

It is not as obvious init_full_scan_done(0) is unneeded.  This sets the
_full_scan_done variable in lvm-globals.c which influences dev-cache.c behavior.
_full_scan_done is initialized to 0 at compile time.  The only place that
sets it to non-zero value is dev-cache.c:_full_scan().

create_toolcontext() calls _init_dev_cache() and _init_filters() - this is
where we could possibly call _full_scan() - wouldn't think so but let's be
sure.  Clearly _init_dev_cache() does not do any actual scanning but just
sets up basic structures and fills in the list of directories or loopfiles
to later scan.  _init_filters() does call into dev-cache.c if
persistent_filter_load() is called.  In persistent_filter_load() we are
opening the .cache file and reading any entries, and note that _read_array()
calls into dev_cache_get().  This call into dev_cache_get() will try to
lookup the device name in the devices cache, and if not found, will insert
it into the devices cache.  At the time of these dev-cache-get() calls,
the devices cache is empty, so each device in the persistent .cache file
will result in _insert() call inside dev_cache_get().  This call could
fail, in which case the subsequent dm_hash_lookup() will fail and
_full_scan(0) will be called.  Will _full_scan(0) call result in
init_full_scan_done(1)?  _full_scan() has this code:
	if (_cache.has_scanned && !dev_scan)
		return;
We know dev_scan == 0, the but what of _cache.has_scanned?  dev_cache_init(),
called earlier, intiialized this to 0.  _cache.has_scanned could be set from
dev_cache_scan().  dev_cache_scan() is called only from
persistent_filter_wipe() and persistent_filter_load().  In
persistent_filter_load(), dev_cache_scan() may be called after _read_array(),
so this doesn't apply.  persistent_filter_wipe() is called from a few of
the lvm tool implementation routines (e.g. pvcreate.c), and dev_iter_create().
None of the tool implementation routines apply, and we wouldn't think any
init code should be iterating through the devices cache by dev_iter_create().
A quick check of the callers confirms this, and so we conclude
_cache.has_scanned must be 0.  Thus, our _full_scan(0) call from dev_cache_get()
will result in a scanning of the devices in the system, as well as
init_full_scan_done(1) being called.  So the init_full_scan_done(0) after
create_toolcontext() is significant.  We didn't think _init_filters() would
result in a scan of the devices in the system, but it does.  But why are
we setting init_full_scan_done(0) here?

Is this a bug?  Why are we setting _full_scan_done = 0 here when we've
already read the config file(s) for the 'scan' and other variables, then
scanned the system for devices as shown above?  Why do we need to act as
though we haven't scanned?  Other places in the code call
init_full_scan_done(0) when they want to indicate devices may have
changed.  But there's nothing indicating to me that something has changed
between the middle of create_toolcontext() where we call _init_filters()
to scan for devices and this call to init_full_scan_done(0).  I have to
conclude it is not needed.

As with init_mirror_in_sync() and perhaps other global variables, a process
that does a sequence of create_toolcontext()/destroy/create may need to
call init_full_scan_done(0) to reset the variable.  A subsequent patch
should address this case, perhaps by exporting a function that resets the
important global variables or perhaps resets the variables at the bottom of
create_toolcontext().

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 tools/lvmcmdline.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/tools/lvmcmdline.c b/tools/lvmcmdline.c
index 5e02057..e1e9826 100644
--- a/tools/lvmcmdline.c
+++ b/tools/lvmcmdline.c
@@ -1091,8 +1091,6 @@ struct cmd_context *init_lvm(unsigned is_static)
 	if (!(cmd = create_toolcontext(_cmdline.the_args, is_static, 0)))
 		return_NULL;
 
-	init_full_scan_done(0);
-
 	return cmd;
 }
 
-- 
1.5.5.1

[PATCH 12/12] Remove init_full_scan_done(0) call after create_toolcontext().

[Alasdair G Kergon]

On Wed, Dec 10, 2008 at 10:26:18PM -0500, Dave Wysochanski wrote:
> _full_scan_done is initialized to 0 at compile time

But as with patch 10, its state should be considered 'undefined' when
create_toolcontext is called.
That initialisation does not include a full device scan (because many
commands that could be run would not require it) and on return from that
function you need it to be set to 0.

If the list of devices is read in from .cache then some code does not
need to scan but other code may still choose to scan.

> Is this a bug?  Why are we setting _full_scan_done = 0 here when we've
> already read the config file(s) for the 'scan' and other variables, then
> scanned the system for devices as shown above?  

We haven't scanned the system at that point.

This scanning code is certainly messy, but it will become 'legacy' when the
udev integration work is done, so it's not worth spending much time on.
(It's a while now since we've had a bug report against this scanning code
so it's best not to change the behaviour of code paths.)

Alasdair
-- 
agk at redhat.com

[PATCH] Create _init_globals() and call from bottom of create_toolcontext().

[Dave Wysochanski]

Move init_full_scan_done(0) and init_mirror_in_sync(0) from init_lvm()
after call to create_toolcontext() to _init_globals(), called from bottom
of create_toolcontext().  No functional change.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/commands/toolcontext.c |    9 +++++++++
 tools/lvmcmdline.c         |    3 ---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/commands/toolcontext.c b/lib/commands/toolcontext.c
index e9ef66c..8e31b9d 100644
--- a/lib/commands/toolcontext.c
+++ b/lib/commands/toolcontext.c
@@ -990,6 +990,13 @@ static void _init_rand(struct cmd_context *cmd)
 	cmd->rand_seed = (unsigned) time(NULL) + (unsigned) getpid();
 }
 
+static void _init_globals(struct cmd_context *cmd)
+{
+	init_full_scan_done(0);
+	init_mirror_in_sync(0);
+
+}
+
 /* Entry point */
 struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
 				       unsigned is_long_lived)
@@ -1090,6 +1097,8 @@ struct cmd_context *create_toolcontext(struct arg *the_args, unsigned is_static,
 
 	_init_rand(cmd);
 
+	_init_globals(cmd);
+
 	cmd->default_settings.cache_vgmetadata = 1;
 	cmd->current_settings = cmd->default_settings;
 
diff --git a/tools/lvmcmdline.c b/tools/lvmcmdline.c
index 66a20ad..e1e9826 100644
--- a/tools/lvmcmdline.c
+++ b/tools/lvmcmdline.c
@@ -1091,9 +1091,6 @@ struct cmd_context *init_lvm(unsigned is_static)
 	if (!(cmd = create_toolcontext(_cmdline.the_args, is_static, 0)))
 		return_NULL;
 
-	init_full_scan_done(0);
-	init_mirror_in_sync(0);
-
 	return cmd;
 }
 
-- 
1.5.5.1

[PATCH] Create _init_globals() and call from bottom of create_toolcontext().

[Alasdair G Kergon]

On Thu, Dec 11, 2008 at 10:15:21PM -0500, Dave Wysochanski wrote:
> Move init_full_scan_done(0) and init_mirror_in_sync(0) from init_lvm()
> after call to create_toolcontext() to _init_globals(), called from bottom
> of create_toolcontext().  No functional change.
 
ACK

[PATCH] Create _init_globals() and call from bottom of create_toolcontext().

[James Cameron]

Move init_full_scan_done(0) and init_mirror_in_sync(0) from init_lvm()
after call to create_toolcontext() to _init_globals(), called from bottom
of create_toolcontext().  No functional change.

Acked-by: James Cameron <james.cameron@hp.com>

-- 
James Cameron
http://ftp.hp.com.au/sigs/jc/

[PATCH 12/12] Remove init_full_scan_done(0) call after create_toolcontext().

[Dave Wysochanski]

On Thu, 2008-12-11 at 23:10 +0000, Alasdair G Kergon wrote:

> > Is this a bug?  Why are we setting _full_scan_done = 0 here when we've
> > already read the config file(s) for the 'scan' and other variables, then
> > scanned the system for devices as shown above?  
> 
> We haven't scanned the system at that point.
> 
> This scanning code is certainly messy, but it will become 'legacy' when the
> udev integration work is done, so it's not worth spending much time on.
> (It's a while now since we've had a bug report against this scanning code
> so it's best not to change the behaviour of code paths.)
> 

You are right.  I don't know where I got lost in that code but I clearly
did because I could not reproduce the scanning from the persistent
filter file reading.  A simpler argument would have just started with 0
or 1 for _full_scan_done and explored alternatives.  As you say though,
it's not worth it!  ;-0