Remove init_full_scan_done(0) call after create_toolcontext().

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().

---
 tools/lvmcmdline.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

index 5e02057..e1e9826 100644
@@ -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

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.

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

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.

---
 lib/commands/toolcontext.c |    9 +++++++++
 tools/lvmcmdline.c         |    3 ---
 2 files changed, 9 insertions(+), 3 deletions(-)

index e9ef66c..8e31b9d 100644
@@ -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;
 

index 66a20ad..e1e9826 100644
@@ -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

ACK

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.

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

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