After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 740651 - [review] dcbw/modprobe-bgo740651: don't use system() to load bonding module and modprobe consolidation
[review] dcbw/modprobe-bgo740651: don't use system() to load bonding module a...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-11-24 19:13 UTC by Dan Williams
Modified: 2015-02-25 14:09 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
0001-platform-don-t-use-system-to-load-bonding-module.patch (1.99 KB, patch)
2014-11-24 19:13 UTC, Dan Williams 		none Details | Review
utils: add nm_utils_modprobe() (2.91 KB, patch)
2015-01-09 11:06 UTC, Thomas Haller 		none Details | Review
core: use nm_utils_modprobe() (3.62 KB, patch)
2015-01-09 11:07 UTC, Thomas Haller 		none Details | Review

Description Dan Williams 2014-11-24 19:13:52 UTC 
Created attachment 291412 [details] [review]
0001-platform-don-t-use-system-to-load-bonding-module.patch

Fixes a compiler warning for unused assignment that we were trying
    and failing to previously shut up.  But beyond that, system() kinda
    sucks for this stuff, so just use nm_utils_find_helper() instead.
Comment 1 Dan Winship 2014-11-24 19:59:07 UTC 
Patch looks fine. There's also:

  src/ppp-manager/nm-ppp-manager.c
  1106:		ignored = system ("/sbin/modprobe ppp_generic");

and there are probably other places where we're currently depending on initscripts having loaded modules for us (bridging? ipoib?) that we may want to fix later. So maybe we should just have nm_utils_modprobe()?
Comment 2 Thomas Haller 2014-11-25 14:05:18 UTC 
LGTM


I think nm_utils_modprobe() is too special. Instead I'd rather add a nm_utils_spawn_sync() which wrapps g_spawn_sync() but does all the stuff we want.
Comment 3 Thomas Haller 2015-01-09 11:06:57 UTC 
Created attachment 294147 [details] [review]
utils: add nm_utils_modprobe()
Comment 4 Thomas Haller 2015-01-09 11:07:03 UTC 
Created attachment 294148 [details] [review]
core: use nm_utils_modprobe()
Comment 5 Thomas Haller 2015-01-09 11:07:53 UTC 
The original patch does not unblock the signals. I think this is complicated enough, to justify a nm_utils_modprobe() function.
Comment 6 Dan Winship 2015-01-12 16:14:16 UTC 
Comment on attachment 294147 [details] [review]
utils: add nm_utils_modprobe()

>+	/* construct the argument list */

If you just used a GPtrArray you could simplify the code by only having to loop once
Comment 7 Dan Winship 2015-01-12 16:14:55 UTC 
The patches basically look good. Although it doesn't really actually matter if the signals get reset, since modprobe just runs and then exits immediately anyway.
Comment 8 Dan Williams 2015-01-13 23:16:55 UTC 
I dropped my patch, and reworked Thomas' first patch for the comments about looping twice with varargs.

Pushed as dcbw/modprobe-bgo740651, please review!
Comment 9 Thomas Haller 2015-01-14 11:10:36 UTC 
(In reply to comment #8)
> I dropped my patch, and reworked Thomas' first patch for the comments about
> looping twice with varargs.
> 
> Pushed as dcbw/modprobe-bgo740651, please review!

I cannot find the branch.
Comment 10 Dan Williams 2015-01-15 15:32:05 UTC 
Sorry, now pushed.
Comment 11 Dan Winship 2015-01-15 21:31:36 UTC 
lgtm
Comment 12 Lubomir Rintel 2015-01-19 12:44:08 UTC 
looks good from here too
Comment 13 Dan Williams 2015-01-19 18:11:43 UTC 
Pushed to git master:

4ad6099b016317563c14648ca6f04fb9e320d032
32625f604bbc45659f16d721e92fb6add9fb1502