diff mbox series

depmod: Introduce a outdir option

Message ID 20230130183324.283162-1-emil.l.velikov@gmail.com (mailing list archive)
State New, archived
Headers show
Series depmod: Introduce a outdir option | expand

Commit Message

Emil Velikov Jan. 30, 2023, 6:33 p.m. UTC
From: Emil Velikov <emil.velikov@collabora.com>

This option is equivalent to basedir, with the small difference being
that's where the meta-data files are generated. In other words, this
allows us to have read-only input modules and modules.dep, while still
being able to generate the meta-data files.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---

Hello team,

Here's a handy feature behind the request at
https://github.com/kmod-project/kmod/issues/13

This is my first time hacking on kmod, so hope I didn't make too many
mistakes :-) There are a few TODO notes below where your input is
greatly appreciated.

TODO:
 - Add tests - team, any pointers or requests?
 - Split the dirnamelen shorthand tenary operator change - is it worth
   it?
---
 man/depmod.xml | 19 +++++++++++++++++++
 tools/depmod.c | 25 ++++++++++++++++++++++---
 2 files changed, 41 insertions(+), 3 deletions(-)

Comments

Lucas De Marchi Feb. 2, 2023, 7:14 a.m. UTC | #1
On Mon, Jan 30, 2023 at 06:33:24PM +0000, Emil Velikov wrote:
>From: Emil Velikov <emil.velikov@collabora.com>
>
>This option is equivalent to basedir, with the small difference being
>that's where the meta-data files are generated. In other words, this
>allows us to have read-only input modules and modules.dep, while still
>being able to generate the meta-data files.
>
>Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>---
>
>Hello team,
>
>Here's a handy feature behind the request at
>https://github.com/kmod-project/kmod/issues/13
>
>This is my first time hacking on kmod, so hope I didn't make too many
>mistakes :-) There are a few TODO notes below where your input is
>greatly appreciated.
>
>TODO:
> - Add tests - team, any pointers or requests?

yep, please add one that calls depmod with this new option.
Copy and extend one of the tests in testsuite/test-depmod.c

You will have to prepare a "rootfs" under
testsuite/rootfs-pristine/test-depmod/<name-of-the-test>. Make sure to
add the normal files coming from kernel and checking for the output in
the other directory you passed as argument.

> - Split the dirnamelen shorthand tenary operator change - is it worth
>   it?
>---
> man/depmod.xml | 19 +++++++++++++++++++
> tools/depmod.c | 25 ++++++++++++++++++++++---
> 2 files changed, 41 insertions(+), 3 deletions(-)
>
>diff --git a/man/depmod.xml b/man/depmod.xml
>index ea0be27..c53624d 100644
>--- a/man/depmod.xml
>+++ b/man/depmod.xml
>@@ -45,6 +45,7 @@
>     <cmdsynopsis>
>       <command>depmod</command>
>       <arg><option>-b <replaceable>basedir</replaceable></option></arg>
>+      <arg><option>-o <replaceable>outdir</replaceable></option></arg>
>       <arg><option>-e</option></arg>
>       <arg><option>-E <replaceable>Module.symvers</replaceable></option></arg>
>       <arg><option>-F <replaceable>System.map</replaceable></option></arg>
>@@ -151,6 +152,24 @@
>           </para>
>         </listitem>
>       </varlistentry>
>+      <varlistentry>
>+        <term>
>+          <option>-o <replaceable>outdir</replaceable></option>
>+        </term>
>+        <term>
>+          <option>--outdir <replaceable>outdir</replaceable></option>
>+        </term>
>+        <listitem>
>+          <para>
>+            If your modules are stored in a read-only location, you may want to
>+            create the output meta-data files in another location. Setting

that is probably not the only use case, I'd rephrase with something
like:

Set the output directory where depmod will store any generated file.
<replaceable>outdir</replaceable> serves as a root to that location,
similar to how <replaceable>basedir</replaceable> is used. Also this
setting takes precedence and if used together with
<replaceable>basedir</replaceable> it will result in the input being
that directory, but the output being the one set by
<replaceable>outdir</replaceable>.

>+            <replaceable>outdir</replaceable> serves as a root to that location
>+            similar to how we use <replaceable>basedir</replaceable>. Use this
>+            option if you are a distribution vendor who needs to pre-generate
>+            the meta-data files rather than running depmod again later.
>+          </para>
>+        </listitem>
>+      </varlistentry>
>       <varlistentry>
>         <term>
>           <option>-C</option>
>diff --git a/tools/depmod.c b/tools/depmod.c
>index 364b7d4..aaf2327 100644
>--- a/tools/depmod.c
>+++ b/tools/depmod.c
>@@ -58,11 +58,12 @@ static const char *default_cfg_paths[] = {
> 	NULL
> };
>
>-static const char cmdopts_s[] = "aAb:C:E:F:euqrvnP:wmVh";
>+static const char cmdopts_s[] = "aAb:o:C:E:F:euqrvnP:wmVh";
> static const struct option cmdopts[] = {
> 	{ "all", no_argument, 0, 'a' },
> 	{ "quick", no_argument, 0, 'A' },
> 	{ "basedir", required_argument, 0, 'b' },
>+	{ "outdir", required_argument, 0, 'o' },
> 	{ "config", required_argument, 0, 'C' },
> 	{ "symvers", required_argument, 0, 'E' },
> 	{ "filesyms", required_argument, 0, 'F' },
>@@ -104,6 +105,7 @@ static void help(void)
> 		"\n"
> 		"The following options are useful for people managing distributions:\n"
> 		"\t-b, --basedir=DIR    Use an image of a module tree.\n"
>+		"\t-o, --outdir=DIR     The output equivalent of basedir.\n"

Output directory for generated files

<basedir>  is both input and output, so I don't think the comparison is
good enough.


I think making sure this is working as desired with at least one test
would be good, but overall looks goot to me.

thanks
Lucas De Marchi

> 		"\t-F, --filesyms=FILE  Use the file instead of the\n"
> 		"\t                     current kernel symbols.\n"
> 		"\t-E, --symvers=FILE   Use Module.symvers file to check\n"
>@@ -467,6 +469,8 @@ struct cfg {
> 	const char *kversion;
> 	char dirname[PATH_MAX];
> 	size_t dirnamelen;
>+	char outdirname[PATH_MAX];
>+	size_t outdirnamelen;
> 	char sym_prefix;
> 	uint8_t check_symvers;
> 	uint8_t print_unknown;
>@@ -2576,7 +2580,7 @@ static int depmod_output(struct depmod *depmod, FILE *out)
> 		{ "modules.devname", output_devname },
> 		{ }
> 	};
>-	const char *dname = depmod->cfg->dirname;
>+	const char *dname = depmod->cfg->outdirname;
> 	int dfd, err = 0;
> 	struct timeval tv;
>
>@@ -2585,6 +2589,11 @@ static int depmod_output(struct depmod *depmod, FILE *out)
> 	if (out != NULL)
> 		dfd = -1;
> 	else {
>+		err = mkdir_p(dname, strlen(dname), 0755);
>+		if (err < 0) {
>+			CRIT("could not create directory %s: %m\n", dname);
>+			return err;
>+		}
> 		dfd = open(dname, O_RDONLY);
> 		if (dfd < 0) {
> 			err = -errno;
>@@ -2898,6 +2907,7 @@ static int do_depmod(int argc, char *argv[])
> 	FILE *out = NULL;
> 	int err = 0, all = 0, maybe_all = 0, n_config_paths = 0;
> 	_cleanup_free_ char *root = NULL;
>+	_cleanup_free_ char *out_root = NULL;
> 	_cleanup_free_ const char **config_paths = NULL;
> 	const char *system_map = NULL;
> 	const char *module_symvers = NULL;
>@@ -2927,6 +2937,11 @@ static int do_depmod(int argc, char *argv[])
> 				free(root);
> 			root = path_make_absolute_cwd(optarg);
> 			break;
>+		case 'o':
>+			if (out_root)
>+				free(out_root);
>+			out_root = path_make_absolute_cwd(optarg);
>+			break;
> 		case 'C': {
> 			size_t bytes = sizeof(char *) * (n_config_paths + 2);
> 			void *tmp = realloc(config_paths, bytes);
>@@ -3009,7 +3024,11 @@ static int do_depmod(int argc, char *argv[])
>
> 	cfg.dirnamelen = snprintf(cfg.dirname, PATH_MAX,
> 				  "%s/lib/modules/%s",
>-				  root == NULL ? "" : root, cfg.kversion);
>+				  root ?: "", cfg.kversion);
>+
>+	cfg.outdirnamelen = snprintf(cfg.outdirname, PATH_MAX,
>+				     "%s/lib/modules/%s",
>+				     out_root ?: (root ?: ""), cfg.kversion);
>
> 	if (optind == argc)
> 		all = 1;
>-- 
>2.39.1
>
diff mbox series

Patch

diff --git a/man/depmod.xml b/man/depmod.xml
index ea0be27..c53624d 100644
--- a/man/depmod.xml
+++ b/man/depmod.xml
@@ -45,6 +45,7 @@ 
     <cmdsynopsis>
       <command>depmod</command>
       <arg><option>-b <replaceable>basedir</replaceable></option></arg>
+      <arg><option>-o <replaceable>outdir</replaceable></option></arg>
       <arg><option>-e</option></arg>
       <arg><option>-E <replaceable>Module.symvers</replaceable></option></arg>
       <arg><option>-F <replaceable>System.map</replaceable></option></arg>
@@ -151,6 +152,24 @@ 
           </para>
         </listitem>
       </varlistentry>
+      <varlistentry>
+        <term>
+          <option>-o <replaceable>outdir</replaceable></option>
+        </term>
+        <term>
+          <option>--outdir <replaceable>outdir</replaceable></option>
+        </term>
+        <listitem>
+          <para>
+            If your modules are stored in a read-only location, you may want to
+            create the output meta-data files in another location. Setting
+            <replaceable>outdir</replaceable> serves as a root to that location
+            similar to how we use <replaceable>basedir</replaceable>. Use this
+            option if you are a distribution vendor who needs to pre-generate
+            the meta-data files rather than running depmod again later.
+          </para>
+        </listitem>
+      </varlistentry>
       <varlistentry>
         <term>
           <option>-C</option>
diff --git a/tools/depmod.c b/tools/depmod.c
index 364b7d4..aaf2327 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -58,11 +58,12 @@  static const char *default_cfg_paths[] = {
 	NULL
 };
 
-static const char cmdopts_s[] = "aAb:C:E:F:euqrvnP:wmVh";
+static const char cmdopts_s[] = "aAb:o:C:E:F:euqrvnP:wmVh";
 static const struct option cmdopts[] = {
 	{ "all", no_argument, 0, 'a' },
 	{ "quick", no_argument, 0, 'A' },
 	{ "basedir", required_argument, 0, 'b' },
+	{ "outdir", required_argument, 0, 'o' },
 	{ "config", required_argument, 0, 'C' },
 	{ "symvers", required_argument, 0, 'E' },
 	{ "filesyms", required_argument, 0, 'F' },
@@ -104,6 +105,7 @@  static void help(void)
 		"\n"
 		"The following options are useful for people managing distributions:\n"
 		"\t-b, --basedir=DIR    Use an image of a module tree.\n"
+		"\t-o, --outdir=DIR     The output equivalent of basedir.\n"
 		"\t-F, --filesyms=FILE  Use the file instead of the\n"
 		"\t                     current kernel symbols.\n"
 		"\t-E, --symvers=FILE   Use Module.symvers file to check\n"
@@ -467,6 +469,8 @@  struct cfg {
 	const char *kversion;
 	char dirname[PATH_MAX];
 	size_t dirnamelen;
+	char outdirname[PATH_MAX];
+	size_t outdirnamelen;
 	char sym_prefix;
 	uint8_t check_symvers;
 	uint8_t print_unknown;
@@ -2576,7 +2580,7 @@  static int depmod_output(struct depmod *depmod, FILE *out)
 		{ "modules.devname", output_devname },
 		{ }
 	};
-	const char *dname = depmod->cfg->dirname;
+	const char *dname = depmod->cfg->outdirname;
 	int dfd, err = 0;
 	struct timeval tv;
 
@@ -2585,6 +2589,11 @@  static int depmod_output(struct depmod *depmod, FILE *out)
 	if (out != NULL)
 		dfd = -1;
 	else {
+		err = mkdir_p(dname, strlen(dname), 0755);
+		if (err < 0) {
+			CRIT("could not create directory %s: %m\n", dname);
+			return err;
+		}
 		dfd = open(dname, O_RDONLY);
 		if (dfd < 0) {
 			err = -errno;
@@ -2898,6 +2907,7 @@  static int do_depmod(int argc, char *argv[])
 	FILE *out = NULL;
 	int err = 0, all = 0, maybe_all = 0, n_config_paths = 0;
 	_cleanup_free_ char *root = NULL;
+	_cleanup_free_ char *out_root = NULL;
 	_cleanup_free_ const char **config_paths = NULL;
 	const char *system_map = NULL;
 	const char *module_symvers = NULL;
@@ -2927,6 +2937,11 @@  static int do_depmod(int argc, char *argv[])
 				free(root);
 			root = path_make_absolute_cwd(optarg);
 			break;
+		case 'o':
+			if (out_root)
+				free(out_root);
+			out_root = path_make_absolute_cwd(optarg);
+			break;
 		case 'C': {
 			size_t bytes = sizeof(char *) * (n_config_paths + 2);
 			void *tmp = realloc(config_paths, bytes);
@@ -3009,7 +3024,11 @@  static int do_depmod(int argc, char *argv[])
 
 	cfg.dirnamelen = snprintf(cfg.dirname, PATH_MAX,
 				  "%s/lib/modules/%s",
-				  root == NULL ? "" : root, cfg.kversion);
+				  root ?: "", cfg.kversion);
+
+	cfg.outdirnamelen = snprintf(cfg.outdirname, PATH_MAX,
+				     "%s/lib/modules/%s",
+				     out_root ?: (root ?: ""), cfg.kversion);
 
 	if (optind == argc)
 		all = 1;