diff mbox series

[OpenWrt-Devel,OpenWrt-Devel,V3,2/2] kmodloader: added -a arg to modprobe

Message ID 20191022090743.1487-1-G.M0N3Y.2503@gmail.com
State Superseded, archived
Headers show
Series None | expand

Commit Message

Gerard Ryan Oct. 22, 2019, 9:07 a.m. UTC 
-a treats all non-op trailing arguments as module names
and attempts to load all of them. This behaviour mirrors the behaviour
of the respective -a in /tools/modprobe.c from https://git.kernel.org.

This is primarily to satiate the multiple modules passed by
docker/libnetwork.

Signed-off-by: Gerard Ryan <G.M0N3Y.2503@gmail.com>
---
Compile tested: x86_x64, Hyper-V, OpenWrt Master
Run tested: x86_x64, Hyper-V, OpenWrt Master

You can also find this patch on GitHub if you prefer.
https://github.com/G-M0N3Y-2503/openwrt-ubox-mirror/tree/feature_extend_modprobe_options

Since https://patchwork.ozlabs.org/patch/1175792/ I adjusted some whitespace to indent more consistently and split the patch by the args they implement.
Since https://patchwork.ozlabs.org/patch/1179955/ I reworded the commit message to explain the functionality of -a

 kmodloader.c | 68 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 30 deletions(-)

--
2.17.1

Comments

Gerard Ryan Dec. 21, 2019, 1:32 a.m. UTC | #1 
Does anyone have any tips on how to expedite the review of this patch?
Am I missing something or is the patch malformed?

Thanks in advance,
Gerard

On Tue, Oct 22, 2019 at 7:08 PM Gerard Ryan <g.m0n3y.2503@gmail.com> wrote:
>
> -a treats all non-op trailing arguments as module names
> and attempts to load all of them. This behaviour mirrors the behaviour
> of the respective -a in /tools/modprobe.c from https://git.kernel.org.
>
> This is primarily to satiate the multiple modules passed by
> docker/libnetwork.
>
> Signed-off-by: Gerard Ryan <G.M0N3Y.2503@gmail.com>
> ---
> Compile tested: x86_x64, Hyper-V, OpenWrt Master
> Run tested: x86_x64, Hyper-V, OpenWrt Master
>
> You can also find this patch on GitHub if you prefer.
> https://github.com/G-M0N3Y-2503/openwrt-ubox-mirror/tree/feature_extend_modprobe_options
>
> Since https://patchwork.ozlabs.org/patch/1175792/ I adjusted some whitespace to indent more consistently and split the patch by the args they implement.
> Since https://patchwork.ozlabs.org/patch/1179955/ I reworded the commit message to explain the functionality of -a
>
>  kmodloader.c | 68 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 30 deletions(-)
>
> diff --git a/kmodloader.c b/kmodloader.c
> index 07b6700..838bc8c 100644
> --- a/kmodloader.c
> +++ b/kmodloader.c
> @@ -681,6 +681,7 @@ static int print_modprobe_usage(void)
>         ULOG_INFO(
>                 "Usage:\n"
>                 "\tmodprobe [-q] [-v] filename\n"
> +               "\tmodprobe -a [-q] [-v] filename [filename...]\n"
>         );
>
>         return -1;
> @@ -854,16 +855,20 @@ static int main_modinfo(int argc, char **argv)
>
>  static int main_modprobe(int argc, char **argv)
>  {
> +       int exit_code = EXIT_SUCCESS;
>         struct module_node *mn;
>         struct module *m;
> -       char *name;
> -       char *mod = NULL;
> +       int load_fail;
>         int log_level = LOG_WARNING;
>         int opt;
>         bool quiet = false;
> +       bool use_all = false;
>
> -       while ((opt = getopt(argc, argv, "qv")) != -1 ) {
> +       while ((opt = getopt(argc, argv, "aqv")) != -1 ) {
>                 switch (opt) {
> +                       case 'a':
> +                               use_all = true;
> +                               break;
>                         case 'q': /* shhhh! */
>                                 quiet = true;
>                                 break;
> @@ -882,48 +887,51 @@ static int main_modprobe(int argc, char **argv)
>         /* after print_modprobe_usage() so it won't be filtered out */
>         ulog_threshold(log_level);
>
> -       mod = argv[optind];
> -
>         if (scan_module_folders())
>                 return -1;
>
>         if (scan_loaded_modules())
>                 return -1;
>
> -       name = get_module_name(mod);
> -       m = find_module(name);
> -       if (m && m->state == LOADED) {
> -               if (!quiet)
> -                       ULOG_ERR("%s is already loaded\n", name);
> -               return 0;
> -       } else if (!m) {
> -               if (!quiet)
> -                       ULOG_ERR("failed to find a module named %s\n", name);
> -               return -1;
> -       } else {
> -               int fail;
> +       do {
> +               char *name;
>
> -               m->state = PROBE;
> +               name = get_module_name(argv[optind]);
> +               m = find_module(name);
>
> -               fail = load_modprobe(true);
> +               if (m && m->state == LOADED) {
> +                       if (!quiet)
> +                               ULOG_INFO("%s is already loaded\n", name);
> +               } else if (!m) {
> +                       if (!quiet)
> +                               ULOG_ERR("failed to find a module named %s\n", name);
> +                       exit_code = EXIT_FAILURE;
> +               } else {
> +                       m->state = PROBE;
> +               }
>
> -               if (fail) {
> -                       ULOG_ERR("%d module%s could not be probed\n",
> -                                fail, (fail == 1) ? ("") : ("s"));
> +               optind++;
> +       } while (use_all && optind < argc);
>
> -                       avl_for_each_element(&modules, mn, avl) {
> -                               if (mn->is_alias)
> -                                       continue;
> -                               m = mn->m;
> -                               if ((m->state == PROBE) || m->error)
> -                                       ULOG_ERR("- %s\n", m->name);
> -                       }
> +       load_fail = load_modprobe(true);
> +       if (load_fail) {
> +               ULOG_ERR("%d module%s could not be probed\n",
> +                        load_fail, (load_fail == 1) ? ("") : ("s"));
> +
> +               avl_for_each_element(&modules, mn, avl) {
> +                       if (mn->is_alias)
> +                               continue;
> +                       m = mn->m;
> +                       if ((m->state == PROBE) || m->error)
> +                               ULOG_ERR("- %s\n", m->name);
>                 }
> +
> +               exit_code = EXIT_FAILURE;
>         }
>
>         free_modules();
>
> -       return 0;
> +       return exit_code;
>  }
>
>  static int main_loader(int argc, char **argv)
> --
> 2.17.1
>
Hans Dedecker Dec. 27, 2019, 3:30 p.m. UTC | #2 
On Sat, Dec 21, 2019 at 2:32 AM Gerard Ryan <g.m0n3y.2503@gmail.com> wrote:
>
> Does anyone have any tips on how to expedite the review of this patch?
> Am I missing something or is the patch malformed?
Patch looks fine; some style comments inline

Hans
>
> Thanks in advance,
> Gerard
>
> On Tue, Oct 22, 2019 at 7:08 PM Gerard Ryan <g.m0n3y.2503@gmail.com> wrote:
> >
> > -a treats all non-op trailing arguments as module names
> > and attempts to load all of them. This behaviour mirrors the behaviour
> > of the respective -a in /tools/modprobe.c from https://git.kernel.org.
> >
> > This is primarily to satiate the multiple modules passed by
> > docker/libnetwork.
> >
> > Signed-off-by: Gerard Ryan <G.M0N3Y.2503@gmail.com>
> > ---
> > Compile tested: x86_x64, Hyper-V, OpenWrt Master
> > Run tested: x86_x64, Hyper-V, OpenWrt Master
> >
> > You can also find this patch on GitHub if you prefer.
> > https://github.com/G-M0N3Y-2503/openwrt-ubox-mirror/tree/feature_extend_modprobe_options
> >
> > Since https://patchwork.ozlabs.org/patch/1175792/ I adjusted some whitespace to indent more consistently and split the patch by the args they implement.
> > Since https://patchwork.ozlabs.org/patch/1179955/ I reworded the commit message to explain the functionality of -a
> >
> >  kmodloader.c | 68 +++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 38 insertions(+), 30 deletions(-)
> >
> > diff --git a/kmodloader.c b/kmodloader.c
> > index 07b6700..838bc8c 100644
> > --- a/kmodloader.c
> > +++ b/kmodloader.c
> > @@ -681,6 +681,7 @@ static int print_modprobe_usage(void)
> >         ULOG_INFO(
> >                 "Usage:\n"
> >                 "\tmodprobe [-q] [-v] filename\n"
> > +               "\tmodprobe -a [-q] [-v] filename [filename...]\n"
> >         );
> >
> >         return -1;
> > @@ -854,16 +855,20 @@ static int main_modinfo(int argc, char **argv)
> >
> >  static int main_modprobe(int argc, char **argv)
> >  {
> > +       int exit_code = EXIT_SUCCESS;
Group this line with the other int parameter declarations below; for
alignment with other functions like lsmod and rmmod I prefer to set
exit_code to 0.
Another alternative is to change this for all functions but then in a
separate patch.
> >         struct module_node *mn;
> >         struct module *m;
> > -       char *name;
> > -       char *mod = NULL;
> > +       int load_fail;
> >         int log_level = LOG_WARNING;
> >         int opt;
> >         bool quiet = false;
> > +       bool use_all = false;
> >
> > -       while ((opt = getopt(argc, argv, "qv")) != -1 ) {
> > +       while ((opt = getopt(argc, argv, "aqv")) != -1 ) {
> >                 switch (opt) {
> > +                       case 'a':
> > +                               use_all = true;
> > +                               break;
> >                         case 'q': /* shhhh! */
> >                                 quiet = true;
> >                                 break;
> > @@ -882,48 +887,51 @@ static int main_modprobe(int argc, char **argv)
> >         /* after print_modprobe_usage() so it won't be filtered out */
> >         ulog_threshold(log_level);
> >
> > -       mod = argv[optind];
> > -
> >         if (scan_module_folders())
> >                 return -1;
> >
> >         if (scan_loaded_modules())
> >                 return -1;
> >
> > -       name = get_module_name(mod);
> > -       m = find_module(name);
> > -       if (m && m->state == LOADED) {
> > -               if (!quiet)
> > -                       ULOG_ERR("%s is already loaded\n", name);
> > -               return 0;
> > -       } else if (!m) {
> > -               if (!quiet)
> > -                       ULOG_ERR("failed to find a module named %s\n", name);
> > -               return -1;
> > -       } else {
> > -               int fail;
> > +       do {
> > +               char *name;
> >
> > -               m->state = PROBE;
> > +               name = get_module_name(argv[optind]);
> > +               m = find_module(name);
> >
> > -               fail = load_modprobe(true);
> > +               if (m && m->state == LOADED) {
> > +                       if (!quiet)
> > +                               ULOG_INFO("%s is already loaded\n", name);
> > +               } else if (!m) {
> > +                       if (!quiet)
> > +                               ULOG_ERR("failed to find a module named %s\n", name);
> > +                       exit_code = EXIT_FAILURE;
Same as above use -1 for alignment with other functions
> > +               } else {
> > +                       m->state = PROBE;
> > +               }
> >
> > -               if (fail) {
> > -                       ULOG_ERR("%d module%s could not be probed\n",
> > -                                fail, (fail == 1) ? ("") : ("s"));
> > +               optind++;
> > +       } while (use_all && optind < argc);
> >
> > -                       avl_for_each_element(&modules, mn, avl) {
> > -                               if (mn->is_alias)
> > -                                       continue;
> > -                               m = mn->m;
> > -                               if ((m->state == PROBE) || m->error)
> > -                                       ULOG_ERR("- %s\n", m->name);
> > -                       }
> > +       load_fail = load_modprobe(true);
> > +       if (load_fail) {
> > +               ULOG_ERR("%d module%s could not be probed\n",
> > +                        load_fail, (load_fail == 1) ? ("") : ("s"));
> > +
> > +               avl_for_each_element(&modules, mn, avl) {
> > +                       if (mn->is_alias)
> > +                               continue;
> > +                       m = mn->m;
> > +                       if ((m->state == PROBE) || m->error)
> > +                               ULOG_ERR("- %s\n", m->name);
> >                 }
> > +
> > +               exit_code = EXIT_FAILURE;
Same as above use -1 for alignment with other functions
> >         }
> >
> >         free_modules();
> >
> > -       return 0;
> > +       return exit_code;
> >  }
> >
> >  static int main_loader(int argc, char **argv)
> > --
> > 2.17.1
> >
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff mbox series

Patch

diff --git a/kmodloader.c b/kmodloader.c
index 07b6700..838bc8c 100644
--- a/kmodloader.c
+++ b/kmodloader.c
@@ -681,6 +681,7 @@  static int print_modprobe_usage(void)
 	ULOG_INFO(
 		"Usage:\n"
 		"\tmodprobe [-q] [-v] filename\n"
+		"\tmodprobe -a [-q] [-v] filename [filename...]\n"
 	);

 	return -1;
@@ -854,16 +855,20 @@  static int main_modinfo(int argc, char **argv)

 static int main_modprobe(int argc, char **argv)
 {
+	int exit_code = EXIT_SUCCESS;
 	struct module_node *mn;
 	struct module *m;
-	char *name;
-	char *mod = NULL;
+	int load_fail;
 	int log_level = LOG_WARNING;
 	int opt;
 	bool quiet = false;
+	bool use_all = false;

-	while ((opt = getopt(argc, argv, "qv")) != -1 ) {
+	while ((opt = getopt(argc, argv, "aqv")) != -1 ) {
 		switch (opt) {
+			case 'a':
+				use_all = true;
+				break;
 			case 'q': /* shhhh! */
 				quiet = true;
 				break;
@@ -882,48 +887,51 @@  static int main_modprobe(int argc, char **argv)
 	/* after print_modprobe_usage() so it won't be filtered out */
 	ulog_threshold(log_level);

-	mod = argv[optind];
-
 	if (scan_module_folders())
 		return -1;

 	if (scan_loaded_modules())
 		return -1;

-	name = get_module_name(mod);
-	m = find_module(name);
-	if (m && m->state == LOADED) {
-		if (!quiet)
-			ULOG_ERR("%s is already loaded\n", name);
-		return 0;
-	} else if (!m) {
-		if (!quiet)
-			ULOG_ERR("failed to find a module named %s\n", name);
-		return -1;
-	} else {
-		int fail;
+	do {
+		char *name;

-		m->state = PROBE;
+		name = get_module_name(argv[optind]);
+		m = find_module(name);

-		fail = load_modprobe(true);
+		if (m && m->state == LOADED) {
+			if (!quiet)
+				ULOG_INFO("%s is already loaded\n", name);
+		} else if (!m) {
+			if (!quiet)
+				ULOG_ERR("failed to find a module named %s\n", name);
+			exit_code = EXIT_FAILURE;
+		} else {
+			m->state = PROBE;
+		}

-		if (fail) {
-			ULOG_ERR("%d module%s could not be probed\n",
-			         fail, (fail == 1) ? ("") : ("s"));
+		optind++;
+	} while (use_all && optind < argc);

-			avl_for_each_element(&modules, mn, avl) {
-				if (mn->is_alias)
-					continue;
-				m = mn->m;
-				if ((m->state == PROBE) || m->error)
-					ULOG_ERR("- %s\n", m->name);
-			}
+	load_fail = load_modprobe(true);
+	if (load_fail) {
+		ULOG_ERR("%d module%s could not be probed\n",
+		         load_fail, (load_fail == 1) ? ("") : ("s"));
+
+		avl_for_each_element(&modules, mn, avl) {
+			if (mn->is_alias)
+				continue;
+			m = mn->m;
+			if ((m->state == PROBE) || m->error)
+				ULOG_ERR("- %s\n", m->name);
 		}
+
+		exit_code = EXIT_FAILURE;
 	}

 	free_modules();

-	return 0;
+	return exit_code;
 }

 static int main_loader(int argc, char **argv)