Created attachment 242335 [details] [review]
Check environment variable for overriding libexecdir

With this patch, one can run NM out of the built tree without having to install e. g. nm-dhcp-helper (the installed system binary doesn't work as this was renamed after 0.9.8.0 from nm-dhcp-client.action). Aside from the renaming, this also ensure to test the latest version of the helpers from trunk.

It introduces the new nm_utils_helper_path() into libnm-utils; if you'd prefer keeping this private, I can also look for a different place, but it might be useful for callouts and even clients, too?

In the same vein, I'd like to introduce a nm_utils_config_path() to allow overriding NMCONFDIR, and nm_utils_state_path() to allow overriding NMSTATEDIR. With that, automatic tests can use private dirs instead of clobbering the system production dirs.

An automatic test suite could alternatively parse these variables from Makefile, re-exec itself under "unshare -m" and put tmpfs mounts over these dirs. But the above is more flexible and may also be useful for manual testing; before I work on patches I'll wait on your opinion about these, though.

Thanks for considering!

I guess I'd rather have daemon arguments for this isntead of environment variables, and also I dont' think we should have new API in libnm-util for it since these things will only be used by the daemon itself, not by clients.  Does that sound OK?

Basically, these things are only used when testing the daemon, which is one reason I don't think we should have the new API in libnm-util.  WRT to the env variable vs. CLI options, I feel that the CLI options are more explicit; I don't mind having a global around somewhere for them, and then putting the path helper in src/NetworkManagerUtils.c.

Also, is there some reason to allow multiple paths?  If we're just trying to override libexecdir, one path should be sufficient, right?

(In reply to comment #3)
> I guess I'd rather have daemon arguments for this isntead of environment
> variables, and also I dont' think we should have new API in libnm-util for it
> since these things will only be used by the daemon itself, not by clients. 
> Does that sound OK?

Sure! I'll rework the patch accordingly.

> Also, is there some reason to allow multiple paths?  If we're just trying to
> override libexecdir, one path should be sufficient, right?

In the source tree, the helpers are distributed across several directories: src/dhcp-manager/ and callouts/ at least (there might be more)

(In reply to comment #4)
> (In reply to comment #3)
> > I guess I'd rather have daemon arguments for this isntead of environment
> > variables, and also I dont' think we should have new API in libnm-util for it
> > since these things will only be used by the daemon itself, not by clients. 
> > Does that sound OK?
> 
> Sure! I'll rework the patch accordingly.
> 
> > Also, is there some reason to allow multiple paths?  If we're just trying to
> > override libexecdir, one path should be sufficient, right?
> 
> In the source tree, the helpers are distributed across several directories:
> src/dhcp-manager/ and callouts/ at least (there might be more)

Oh, I get it now.  But instead of paths like that, would it be simpler to just -DBUILDEXECDIR="\"$(absbuilddir)\"" in each dir's Makefile.am where a helper is used and just use that to construct the helper path instead?  Then some kind of --test-use-builddir-helpers could be used to find them locally instead?  Just a thought.

(In reply to comment #5)
> Oh, I get it now.  But instead of paths like that, would it be simpler to just
> -DBUILDEXECDIR="\"$(absbuilddir)\"" in each dir's Makefile.am where a helper is
> used and just use that to construct the helper path instead?  Then some kind of
> --test-use-builddir-helpers could be used to find them locally instead?  Just a
> thought.

I think that an environment variable (with the path or list of paths) is more convenient for path overrides and prevents you from having to pollute the argument list with test-only stuff.

But I definitely agree that configuration paths belong to the list of command line arguments, as opposed to paths to libraries and executables. I see this as an exception, as anyone might want to try out (even installed) NM with alternative set of configuration files.

When I was thinking about the NM testing, I realised that I don't want to run *the NetworkManager binary* for the tests but that I would actually convert the main() function to a generic run() function or even NetworkManager/NMDaemon object that could be set up and then run. With this alternative approach it would be easy to leave the NetworkManager binary as is and create a separate binary to run everything from the build directory.

(In reply to comment #1)
> With this patch, one can run NM out of the built tree without having to install
> e. g. nm-dhcp-helper (the installed system binary doesn't work as this was
> renamed after 0.9.8.0 from nm-dhcp-client.action). Aside from the renaming,
> this also ensure to test the latest version of the helpers from trunk.

Actually, I'm not sure how much is the DHCP helper good for in-tree automatic tests as it is used with the DHCP client to listen on real interfaces. I was going to use a fake DHCP plugin (similar to NMFakePlatform recently pushed to master) for DHCP tests.

> It introduces the new nm_utils_helper_path() into libnm-utils; if you'd prefer
> keeping this private, I can also look for a different place, but it might be
> useful for callouts and even clients, too?

You already answered to dcbw's comments...

There are things that you do want to be used with clients (or specific client builds, at the least), especially the private bus socket path. It would be nice if you could run a specifically configured NetworkManager for testing in addition to the system NetworkManager instance. It would also be nice if you could run a "fake" instance of NetworkManager with NMFakePlatform in it and all those fake DHCP, fake NDP, etc, and at the same time run e.g. nmcli or python with libnm-glib against it. But this is really a use case for environment variables as you may run several tools in row and want all of them to share the path configuration.

(In reply to comment #7)
> Actually, I'm not sure how much is the DHCP helper good for in-tree automatic
> tests as it is used with the DHCP client to listen on real interfaces. I was
> going to use a fake DHCP plugin (similar to NMFakePlatform recently pushed to
> master) for DHCP tests.

I'm writing system integration tests, so I actually do want the real NetworkManager, real dhclient, etc. We use mac80211_hwsim and veth devices, which are "real" from the kernel's and NM's point of view. Also, if you want to do a manual test you still want to use the real dhcp plugin instead of a fake one.

(In reply to comment #6)
> (In reply to comment #5)
> > Oh, I get it now.  But instead of paths like that, would it be simpler to just
> > -DBUILDEXECDIR="\"$(absbuilddir)\"" in each dir's Makefile.am where a helper is
> > used and just use that to construct the helper path instead?  Then some kind of
> > --test-use-builddir-helpers could be used to find them locally instead?  Just a
> > thought.
> 
> I think that an environment variable (with the path or list of paths) is more
> convenient for path overrides and prevents you from having to pollute the
> argument list with test-only stuff.

Ok, I guess I buy that.  However, I still think the automatic BUILDDIR stuff is better since then you don't have to list a bunch of different directories, it's all handled for you.  NM knows what build directories the binaries are in already, so we might as well use that information instead of making the user string them all together.

> But I definitely agree that configuration paths belong to the list of command
> line arguments, as opposed to paths to libraries and executables. I see this as
> an exception, as anyone might want to try out (even installed) NM with
> alternative set of configuration files.

Yeah, we already have --config-dir in git master to point NM somewhere else.

For the client side of things, the private bus path is hardcoded, partly because we want to ensure that clients aren't misdirected somewhere else than the root-controlled NM.  I'm not sure that should be easily changed in production builds, though I do see how it's useful for testing.

Am I just overthinking any potential security issues here?  Or should we go with a testing library (like what we have already LIBNM_GLIB_TEST but which we should kill) that's exactly the same as the real library but which a tester could use with LD_PRELOAD maybe?

(In reply to comment #10)
> Am I just overthinking any potential security issues here?

Are they real? Most tools can be fooled with bad environment to do just *anything* (think of stuff like PATH, LD_PRELOAD, PYTHONPATH, etc.). Security tools like sudo already sanitize the environment to only pass a limited set of variables.

> Or should we go
> with a testing library (like what we have already LIBNM_GLIB_TEST but which we
> should kill) that's exactly the same as the real library but which a tester
> could use with LD_PRELOAD maybe?

See. You're using an environment variable to avoid having to use another environment variable for security reasons. Are there tools that only check for known dangerous variables? I'd say that would constitute a security risk itself. You never know your list of dangerous variables is complete.

(In reply to comment #5)
> Oh, I get it now.  But instead of paths like that, would it be simpler to just
> -DBUILDEXECDIR="\"$(absbuilddir)\"" in each dir's Makefile.am where a helper is
> used and just use that to construct the helper path instead?

We can do something like that, yes (not the absolute path, to keep the whole tree relocatable).

> Then some kind of --test-use-builddir-helpers could be used to find them
> locally instead?

I didn't like that very much as this option would be visible in the installed binary as well, but would just be broken. An environment variable does not expose itself in the same way.

An env variable would also still work in the installed version if you point it somewhere else in the original patch; but that would of course not work any more with builtin hardcoded paths, but I don't think that's a problem. I would just like to avoid making it an unnecesary part of the publicly visible CLI.

None of this is security relevant, btw. If you are able to start NetworkManager, you are already root and thus can start it with any kind of LD_LIBRARY_PATH, PATH, LD_PRELOAD, ptrace(), and whatnot.

(In reply to comment #12)
> > Then some kind of --test-use-builddir-helpers could be used to find them
> > locally instead?
> 
> I didn't like that very much as this option would be visible in the installed
> binary as well, but would just be broken.

I must agree that pushing something broken to the resulting binary seems weird to me also.

> An environment variable does not expose itself in the same way.
> 
> An env variable would also still work in the installed version if you point it
> somewhere else in the original patch;

Exactly. It's like a good building block that you can use to point NetworkManager somewhere else. It would be similar to variables like PATH, LD_LIBRARY_PATH, etc and could be used the same way.

> but that would of course not work any
> more with builtin hardcoded paths, but I don't think that's a problem. I would
> just like to avoid making it an unnecesary part of the publicly visible CLI.

And it seems to be a much cleaner solution. And it could potentially be used in other ways.

> None of this is security relevant, btw. If you are able to start
> NetworkManager, you are already root and thus can start it with any kind of
> LD_LIBRARY_PATH, PATH, LD_PRELOAD, ptrace(), and whatnot.

My impression is the same. For the system service environment, systemd is responsible.

Just filed related bug 699774.

(In reply to comment #3)
> Basically, these things are only used when testing the daemon, which is one
> reason I don't think we should have the new API in libnm-util.  WRT to the env
> variable vs. CLI options, I feel that the CLI options are more explicit; I
> don't mind having a global around somewhere for them, and then putting the path
> helper in src/NetworkManagerUtils.c.

I tried that now, but run into some serious trouble. nm-dhcp-helper and the like are not really designed to work with src/NetworkManagerUtils.[hc]. Adding these to the helper's _SOURCES causes a plethora of errors, as these utils depend on a lot of other source files; NetworkManagerUtils.[hc] are not built into library, so one cannot just link that to the helpers either. I think it would be cleaner, and also avoid excess (binary) code duplication to not link the helpers against half of the NM code in src/, and relay the "using callouts from build tree" flag through an environment variable. If we still want to have a CLI option for this, that CLI option then could merely set that variable; BTW, we can use G_OPTION_FLAG_HIDDEN to avoid having this show up in --help.

Created attachment 243482 [details] [review]
Add option for calling helpers from the build tree

This is an alternative patch as discussed above. It puts a global into src/config/nm-config.c (as src/dhcp-manager/* doesn't have an NMConfig object), that's accessible from all places without pulling in extra bloat/dependencies. With that you can call "src/NetworkManager --helpers-from-builddir", and that new option doesn't appear in --help.

This now hardcodes relative paths like

+       -DBUILDEXECDIR=\"src\" \

and

+       -DBUILDEXECDIR=\"src/dhcp-manager\" \

as automake does not have a variable for "relative subdir starting from top_builddir". But I really don't want to use abs_builddir as this would make the tree totally non-relocatable (such as copying binaries into a VM and testing them there).

Comparing both patches, I still find the first one more flexible and elegant, but I don't mind much which approach we use in the end, as long as we get it running from the build tree somehow. :-)

Thank you in advance for review!

(In reply to comment #15)
> (In reply to comment #3)
> > Basically, these things are only used when testing the daemon, which is one
> > reason I don't think we should have the new API in libnm-util.  WRT to the env
> > variable vs. CLI options, I feel that the CLI options are more explicit; I
> > don't mind having a global around somewhere for them, and then putting the path
> > helper in src/NetworkManagerUtils.c.
> 
> I tried that now, but run into some serious trouble. nm-dhcp-helper and the
> like are not really designed to work with src/NetworkManagerUtils.[hc]. Adding
> these to the helper's _SOURCES causes a plethora of errors, as these utils
> depend on a lot of other source files; NetworkManagerUtils.[hc] are not built
> into library,

Dan Winship is AFAIK working on this, see:

https://bugzilla.gnome.org/show_bug.cgi?id=699774#c1

> so one cannot just link that to the helpers either. I think it
> would be cleaner, and also avoid excess (binary) code duplication to not link
> the helpers against half of the NM code in src/, and relay the "using callouts
> from build tree" flag through an environment variable.

So, what do you suggest? Note that static linking is rather economic when you're using a small subset of the library.

> If we still want to have
> a CLI option for this, that CLI option then could merely set that variable;
> BTW, we can use G_OPTION_FLAG_HIDDEN to avoid having this show up in --help.

I still like the env way better because you can add a .sh script that you can source in your shell and the environment would then set up stuff also for other tools in the tree if necessary. But something is better than nothing.

(In reply to comment #12)
> None of this is security relevant, btw. If you are able to start
> NetworkManager, you are already root and thus can start it with any kind of
> LD_LIBRARY_PATH, PATH, LD_PRELOAD, ptrace(), and whatnot.

I'm talking about the client side when I talk about security actually.  If you want to run NM in-tree, then NM needs to create the private dbus sockets too, and those are always in /var/tmp or /var/run.  I was assuming that you'd also want to relocate those as well.  If you don't want to relocate those, and you don't care about changing libnm-glib, then we don't have a problem with security.

But the point about LD_PRELOAD is valid, something can just walk all over libnm-glib with a custom library and doesn't need to twiddle env vars.

Ok, so how about an environment variable like NM_TEST_BASE_DIR which would you'd set to the NM base build directory.

I was going to be clever and suggest some 'sed' magic with ${abs_builddir} and ${abs_top_builddir}, but danw/movedevs converts src/ to a non-recursive build, so we'll need to hardcode the helper paths anyway.  I don't have a problem with that, we might as well just do that in the code.  So for DHCP we'd have something like:

diff --git a/src/dhcp-manager/nm-dhcp-dhclient.c b/src/dhcp-manager/nm-dhcp-dhclient.c
index c2fc707..689fe59 100644
--- a/src/dhcp-manager/nm-dhcp-dhclient.c
+++ b/src/dhcp-manager/nm-dhcp-dhclient.c
@@ -501,11 +501,11 @@ dhclient_start (NMDHCPClient *client,
        GPtrArray *argv = NULL;
        GPid pid = -1;
        GError *error = NULL;
-       const char *iface, *uuid, *system_bus_address;
+       const char *iface, *uuid, *system_bus_address, *test_base_dir;
        char *binary_name, *cmd_str, *pid_file = NULL, *system_bus_address_env = NULL;
        gboolean ipv6, success;
        guint log_domain;
-       char *escaped, *preferred_leasefile_path = NULL;
+       char *escaped, *preferred_leasefile_path = NULL, *helper_path = NULL;
 
        g_return_val_if_fail (priv->pid_file == NULL, -1);
 
@@ -589,7 +589,13 @@ dhclient_start (NMDHCPClient *client,
                        g_ptr_array_add (argv, (gpointer) mode_opt);
        }
        g_ptr_array_add (argv, (gpointer) "-sf");       /* Set script file */
-       g_ptr_array_add (argv, (gpointer) ACTION_SCRIPT_PATH );
+       test_base_dir = getenv ("NM_TEST_BASE_PATH");
+       if (test_base_dir && test_base_dir[0])
+               helper_path = g_strdup_printf ("%s/src/dhcp-manager/nm-dhcp-helper", test_base_dir);
+       else
+               helper_path = g_strdup_printf (LIBEXECDIR "/nm-dhcp-helper");
+
+       g_ptr_array_add (argv, (gpointer) helper_path);
 
        if (pid_file) {
                g_ptr_array_add (argv, (gpointer) "-pf");       /* Set pid file */
@@ -634,6 +640,7 @@ dhclient_start (NMDHCPClient *client,
        }
 
        g_ptr_array_free (argv, TRUE);
+       g_free (helper_path);
        g_free (system_bus_address_env);
        return pid;
 }

yeah, src/dhcp-manager/ is kinda ugly, but these paths aren't going to change very often.  Flat makefiles did kinda screw us on this one, otherwise it'd be a lot easier with some makefile magic.

If the objective is to run from the build tree, then there's a possibly simpler alternate approach:

1) command-line switch --run-from-test-dir which uses argv[0] and g_get_current_dir() to construct the absolute path of the NM program
2) absolute path gets stripped of any ".libs/" and "NetworkManager" and then copied into a global char *test_base_dir
3) any helper scripts use the given test_base_dir to find their helper script similar to how I propose in comment 19

Basically, this is easier for the specific case of running NM completely from its build dir, since the user doesn't have to figure out the paths themselves, they simply use a command-line switch.  I can't really think of a valid use-case where you'd run NM from some directory that's not related to where the helper scripts are, even for testing, since you want to test the same NM and helper scripts together.

(In reply to comment #18)
> (In reply to comment #12)
> > None of this is security relevant, btw. If you are able to start
> > NetworkManager, you are already root and thus can start it with any kind of
> > LD_LIBRARY_PATH, PATH, LD_PRELOAD, ptrace(), and whatnot.
> 
> I'm talking about the client side when I talk about security actually.  If you
> want to run NM in-tree, then NM needs to create the private dbus sockets too,
> and those are always in /var/tmp or /var/run.  I was assuming that you'd also
> want to relocate those as well.

For my part, yes. In some cases it not an option to use system directories.

> If you don't want to relocate those, and you
> don't care about changing libnm-glib, then we don't have a problem with
> security.
> 
> But the point about LD_PRELOAD is valid, something can just walk all over
> libnm-glib with a custom library and doesn't need to twiddle env vars.

Good. AFAIK environment variables were always regarded precious as a whole, so guarding against specific ones won't help.

(In reply to comment #20)
> If the objective is to run from the build tree, then there's a possibly simpler
> alternate approach:
> 
> 1) command-line switch --run-from-test-dir which uses argv[0] and
> g_get_current_dir() to construct the absolute path of the NM program
> 2) absolute path gets stripped of any ".libs/" and "NetworkManager" and then
> copied into a global char *test_base_dir
> 3) any helper scripts use the given test_base_dir to find their helper script
> similar to how I propose in comment 19

I don't think I see the "simpler" attribute of this approach. I believe that it is conceptually bad to introduce testing switches into the whole code. In my opinion it adds unnecessary complexity and reduces test coverage. The paths should be IMO handled in a place in code and a single data structure in NetworkManager code and then just pushed to (or pulled by) NM modules that need it.

And as you stated above, the private bus path will be also altered and that's a typical use case for environment variables. With the right environment variable set, you could use any libnm-glib based tools to access the NetworkManager without a rebuild.

I don't say the private bus path and plugin/helper path need the same handling. You might want to use CLI args for NetworkManager daemon and environment variable for libnm-glib based tools.

What I asked for is...

1) Don't be afraid of environment variables. Security implications are void. Environment handling is *much* easier than command-line arg handling, as it can be done from anywhere if needed.

2) Don't introduce different code paths in many places of the NetworkManager code to distinguish testing versus real system runs. Instead, please use one place in the code to gather all the information whether they come from command-line arguments, environment variables or internal defaults.

With #2 in mind, we can choose arbitrarily between args and env, as it will be easy to switch to the other way.

> Basically, this is easier for the specific case of running NM completely from
> its build dir, since the user doesn't have to figure out the paths themselves,
> they simply use a command-line switch.

A shell script with the right env generated would be just as easy.

> I can't really think of a valid
> use-case where you'd run NM from some directory that's not related to where the
> helper scripts are, even for testing, since you want to test the same NM and
> helper scripts together.

While it is important to achieve the known use cases, I don't think we need to seek more use cases right now just to justify a more generic solution. It was just a feeling (shared with Martin) that env is a more generic solution and that it *might* be better.

I still prefer the env way with a generated script, avoiding pollution of the installed NM binaries with more of the build system's environment (e.g. the paths to specific build subdirs). Separation of build/test stuff from the real system stuff feels like a generally reasonable approach.

If we keep #2 above in mind, we can decide between args and env at any time.

(In reply to comment #18)
> I'm talking about the client side when I talk about security actually.  If you
> want to run NM in-tree, then NM needs to create the private dbus sockets too,
> and those are always in /var/tmp or /var/run.  I was assuming that you'd also
> want to relocate those as well.

Not really; I currently stop the system NM before starting the one in the tree; first, because I want to be as close as possible to the "real" NM, and second, trying to make two NMs work in parallel seems to be both hard and also not really necessary.

(In reply to comment #17)
> So, what do you suggest?

The second attached patch just puts it into NetworkManagerUtils.h, which works fine.

> I still like the env way better because you can add a .sh script that you can
> source in your shell and the environment would then set up stuff also for other
> tools in the tree if necessary. But something is better than nothing.

For that we could do a synthesis of the two patches: Env var based, but without changing libnm-glib, but putting the helper into NetworkManagerUtils.[hc]? (This might require some code shuffling and/or link rearrangements).

(In reply to comment #20)
> If the objective is to run from the build tree, then there's a possibly simpler
> alternate approach:
> 
> 1) command-line switch --run-from-test-dir which uses argv[0] and
> g_get_current_dir() to construct the absolute path of the NM program
> 2) absolute path gets stripped of any ".libs/" and "NetworkManager" and then
> copied into a global char *test_base_dir
> 3) any helper scripts use the given test_base_dir to find their helper script
> similar to how I propose in comment 19

This is already the direction that the second patch takes, except that we'd put the knowledge of the subdirectory from Makefile.am into the .c files. That also works for me.

Now it seems that Pavel and Dan are favoring those two different approaches -- which one shall it be? :-)

(In reply to comment #22)
> Now it seems that Pavel and Dan are favoring those two different approaches --
> which one shall it be? :-)

Dan will do the final decision, hopefully with my remarks in mind ;).

Created attachment 244009 [details] [review]
Add option for running from the build tree

Take three. This is now what Dan suggested in comments 19 and 20, adding a --run-from-build-dir option which defines a $NM_TEST_BASE_PATH env var, and helper paths are constructed based on that.

Pavel, you can also set $NM_TEST_BASE_PATH explicitly of course, instead of calling NetworkManager with --run-from-build-dir.

(In reply to comment #25)
> Pavel, you can also set $NM_TEST_BASE_PATH explicitly of course, instead of
> calling NetworkManager with --run-from-build-dir.

Sounds good!

(In reply to comment #24)
> Created an attachment (id=244009) [details] [review]
> Add option for running from the build tree

Martin, would it be possible to move the test_base_path handling out of the dhcp code?

For example:

+	if (test_base_path && test_base_path[0])
+		helper_path = g_strdup_printf ("%s/src/dhcp-manager/nm-dhcp-helper", test_base_path);
+	else
+		helper_path = g_strdup (ACTION_SCRIPT_PATH);

I don't think it's necessary to handle the condition there at all and the DHCP code should IMO have just one path variable on its input for simplicity and better test coverage. It could be main.c now, later modified in line with bug 699774.

Created attachment 244968 [details] [review]
Add option for running from the build tree

Pawel,

something like this? This drops the env variable and replaces the ACTION_PATH-like macros with global variables (something that Dan proposed earlier already). With the restructured build system this works very well now.

This makes the patch quite a bit simpler, too.

(In reply to comment #28)
> something like this?

Yep, exactly like that.

> This drops the env variable and replaces the
> ACTION_PATH-like macros with global variables (something that Dan proposed
> earlier already).

OK. Let's see how well it works for us and possibly change it later.

> With the restructured build system this works very well now.
> 
> This makes the patch quite a bit simpler, too.

Definitely.

I think the patch is ready to go. Do you have Git access, yet?

(In reply to comment #29)
> Yep, exactly like that.

Great, thanks for the review and guidance.

> I think the patch is ready to go. Do you have Git access, yet?

I have a fd.o account ("martin") for pushing to udisks, upower, systemd, and the like, but I'm not a NM committer. So far I haven't really worked on NM, it just came up as the next thing which needs testing :)

Pushed.

You can request access to NetworkManager if you think it's worth it. Otherwise, I can always push the patches for you.

(In reply to comment #30)
> (In reply to comment #29)
> > Yep, exactly like that.
> 
> Great, thanks for the review and guidance.
> 
> > I think the patch is ready to go. Do you have Git access, yet?
> 
> I have a fd.o account ("martin") for pushing to udisks, upower, systemd, and
> the like, but I'm not a NM committer. So far I haven't really worked on NM, it
> just came up as the next thing which needs testing :)

If you request access, please send me the bug # so I can ACK it for fdo admins.  Then you could start pushing your testing stuff in :)

Done: https://bugs.freedesktop.org/show_bug.cgi?id=64890 (I subscribed both of you).

Of course I'll only do unreviewed commits for trivial stuff and integration test suite additions/maintenance, for bigger changes I'll continue to discuss them on bz first.