[RFC PATCH 0/4] dtc: Add a plugin interface

[RFC PATCH 0/4] dtc: Add a plugin interface

[Andrei Ziureaev]

This is one possible implementation of a plugin interface for dtc.

A plugin interface would allow us to add checks in languages other than
C. Specifically, it would allow dtc to parse dts files, validate them
using DT Schema (which is written in Python), and output dtb files, all
in one command. The command would look something like:

dtc -O dtb -o test.dtb -Lplugins/ \
	-ldt-validate,u,./Documentation/devicetree/bindings \
	-ldt-validate,p,./Documentation/devicetree/bindings/processed-schema.yaml \
	test.dts

DT Schema would also be able to access dts source line information. If
we don't end up having a plugin interface, the alternative would be to
add source annotations to dtc's yaml output.

The plugin interface exports the live tree, making it an ABI. This is
the part I'm most unsure about. Right now, I just cut-and-pasted some
definitions that might be useful for plugins into dtc-plugin.h. This
means the whole live tree structure is exposed and plugins can change it
in any way they want.

I guess a better option would be to make a standard library for live
trees, and have that separate from what dtc uses internally. Some
functions from livetree.c seem useful (although I haven't looked too
much into it yet).

There's also a question of whether we should relicense dtc-plugin.h to
dual BSD to be able call Python code.

Patch 1 fixes a small header issue.
Patch 2 contains an overview of what I have in mind.
Patch 3 isn't very exciting.
Patch 4 is the implementation.

TODO:
- add tests
- improve dtc-plugin.h
- write a DT Schema plugin

Any thoughts would be much appreciated.

Thanks,
Andrei.

Andrei Ziureaev (4):
  dtc: Include stdlib.h in util.h
  dtc: Add plugin documentation and examples
  dtc: Move some definitions into dtc-plugin.h
  dtc: Add a plugin interface

 Documentation/manual.txt         |  74 ++++++++++++
 Makefile                         |  32 ++++-
 dtc-plugin.h                     | 200 +++++++++++++++++++++++++++++++
 dtc.c                            | 139 ++++++++++++++++++++-
 dtc.h                            | 192 ++++++++---------------------
 plugins/example/Makefile.example |  19 +++
 plugins/example/example.c        |  29 +++++
 treesource.c                     |  21 ----
 util.h                           |   1 +
 9 files changed, 540 insertions(+), 167 deletions(-)
 create mode 100644 dtc-plugin.h
 create mode 100644 plugins/example/Makefile.example
 create mode 100644 plugins/example/example.c

-- 
2.17.1

[RFC PATCH 1/4] dtc: Include stdlib.h in util.h

[Andrei Ziureaev]

If used on its own, util.h needs stdlib.h for exit(), malloc() and
realloc().

Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
---
 util.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util.h b/util.h
index 5a4172d..a771b46 100644
--- a/util.h
+++ b/util.h
@@ -2,6 +2,7 @@
 #ifndef UTIL_H
 #define UTIL_H
 
+#include <stdlib.h>
 #include <stdarg.h>
 #include <stdbool.h>
 #include <getopt.h>
-- 
2.17.1

Re: [RFC PATCH 1/4] dtc: Include stdlib.h in util.h

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

On Tue, Jul 21, 2020 at 04:58:57PM +0100, Andrei Ziureaev wrote:
> If used on its own, util.h needs stdlib.h for exit(), malloc() and
> realloc().
> 
> Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>

This one makes sense regardless of the rest, so applied.

> ---
>  util.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/util.h b/util.h
> index 5a4172d..a771b46 100644
> --- a/util.h
> +++ b/util.h
> @@ -2,6 +2,7 @@
>  #ifndef UTIL_H
>  #define UTIL_H
>  
> +#include <stdlib.h>
>  #include <stdarg.h>
>  #include <stdbool.h>
>  #include <getopt.h>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC PATCH 2/4] dtc: Add plugin documentation and examples

[Andrei Ziureaev]

Document the plugin API in Documentation/manual.txt and provide an
example plugin.

Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/manual.txt         | 74 ++++++++++++++++++++++++++++++++
 plugins/example/Makefile.example | 19 ++++++++
 plugins/example/example.c        | 29 +++++++++++++
 3 files changed, 122 insertions(+)
 create mode 100644 plugins/example/Makefile.example
 create mode 100644 plugins/example/example.c

diff --git a/Documentation/manual.txt b/Documentation/manual.txt
index adf5ccb..18624aa 100644
--- a/Documentation/manual.txt
+++ b/Documentation/manual.txt
@@ -10,6 +10,10 @@ I - "dtc", the device tree compiler
     4.1) Overview
     4.2) Properties
     4.3) Labels and References
+    5) Plugins
+    5.1) Loading plugins
+    5.2) Exporting Functionality
+    5.3) Building Plugins
 
 II - The DT block format
     1) Header
@@ -115,6 +119,16 @@ Options:
     -d <dependency_filename>
 	Generate a dependency file during compilation.
 
+    -l, --plugin <plugin name>[,key][,value]
+	Load a plugin and, optionally, pass it an argument.
+		plugin name - the name of the shared object without the extension (can contain a path)
+		key         - the name of the argument
+		value       - can be omitted
+	Example: dtc -lsome-plugin,o,out.dts -lsome-plugin,help -l path/to/another-plugin [...]
+
+    -L, --plugin-dir <arg>
+	The directory in which to search for plugins
+
     -q
 	Quiet: -q suppress warnings, -qq errors, -qqq all
 
@@ -272,6 +286,66 @@ And used in properties, labels may appear before or after any value:
     };
 
 
+5) Plugins
+
+Plugins are shared libraries that DTC loads at runtime using
+
+	dlopen(path, RTLD_NOW | RTLD_GLOBAL | RTLD_DEEPBIND)
+
+RTLD_NOW means they are loaded immediately (right before the parsing
+stage).
+
+RTLD_GLOBAL means their symbols can be used by other plugins.
+
+RTLD_DEEPBIND means the dynamic linker will look for symbols within the
+plugin first, before searching in the main program and other plugins.
+
+All plugins must include the "dtc-plugin.h" header.
+
+
+5.1) Loading plugins
+
+Please, refer to the -l and -L options in the "Command Line" section.
+
+
+5.2) Exporting Functionality
+
+Plugins export functionality through the "EXPORT_FUNCTION(type, fn)"
+macro, where "type" is one of the typedefs from "dtc-plugin.h", and
+"fn" is the function that will be called by DTC. Each type of function
+gets called once for each plugin.
+
+Every plugin must export a function of type "init_fn_t".
+
+"plugins/example/example.c" shows how an init function can be used.
+
+
+5.3) Building Plugins
+
+The command "make plugins" infers the names of the shared library files
+from the names of directories under "plugins/". For example, the
+presence of a
+
+	plugins/example/
+
+directory means that make will try to build
+
+	plugins/example/example.so
+
+from
+
+	plugins/example/example.c
+
+It will then make a symlink
+
+	plugins/example.so
+
+for convenience.
+
+Please, see "plugins/example/Makefile.example" for how to override the
+defaut behavior.
+
+
 
 II - The DT block format
 ========================
diff --git a/plugins/example/Makefile.example b/plugins/example/Makefile.example
new file mode 100644
index 0000000..56d4e17
--- /dev/null
+++ b/plugins/example/Makefile.example
@@ -0,0 +1,19 @@
+# Optional per-plugin makefile
+#
+# Here you can add gcc flags:
+# PLUGIN_CFLAGS_example = -some-flag
+#
+# Add libraries:
+# PLUGIN_LDLIBS_example = -lsome-lib
+#
+# Add files to clean:
+# PLUGIN_CLEANFILES += $(PLUGIN_dir)/example/*.tmp
+#
+# Or override the default rules:
+# $(PLUGIN_dir)/example/example.$(SHAREDLIB_EXT): $(PLUGIN_dir)/example/example.o
+#	@$(VECHO) LD $@
+#	...
+#
+# $(PLUGIN_dir)/example/example.o: $(PLUGIN_dir)/example/example.c
+#	@$(VECHO) CC $@
+#	...
diff --git a/plugins/example/example.c b/plugins/example/example.c
new file mode 100644
index 0000000..3bbf9ea
--- /dev/null
+++ b/plugins/example/example.c
@@ -0,0 +1,29 @@
+#include <stdio.h>
+#include <string.h>
+
+#include "dtc-plugin.h"
+
+#define NAME "example: "
+
+static int init_example(struct plugin_version v, int argc, struct plugin_arg *argv)
+{
+	if (!dtc_plugin_default_version_check(v)) {
+		fprintf(stderr, NAME"Plugin is incompatible with this version of DTC\n");
+		return 1;
+	}
+
+	for (int i = 0; i < argc; i++) {
+		if (strcmp(argv[i].key, "h") == 0
+		 || strcmp(argv[i].key, "help") == 0) {
+			printf(NAME"This is an example plugin. It prints its arguments.\n");
+			return 1;
+		} else {
+			printf(NAME"%s: %s\n", argv[i].key,
+			       argv[i].value ? argv[i].value : "");
+		}
+	}
+
+	printf(NAME"Loaded plugin 'example'\n");
+	return 0;
+}
+EXPORT_FUNCTION(init_fn_t, init_example);
-- 
2.17.1

Re: [RFC PATCH 2/4] dtc: Add plugin documentation and examples

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5892 bytes --]

On Tue, Jul 21, 2020 at 04:58:58PM +0100, Andrei Ziureaev wrote:
> Document the plugin API in Documentation/manual.txt and provide an
> example plugin.
> 
> Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>

So, thanks for starting out with some docs.  However, I don't think
these docs cover the most important things for them to cover: what the
API is for the plugin - what information goes in, what information
goes out, and what all the supported entry points are.  Understanding
the data model of the plugin would make reviewing the rest of the
details much easier.

> ---
>  Documentation/manual.txt         | 74 ++++++++++++++++++++++++++++++++
>  plugins/example/Makefile.example | 19 ++++++++
>  plugins/example/example.c        | 29 +++++++++++++
>  3 files changed, 122 insertions(+)
>  create mode 100644 plugins/example/Makefile.example
>  create mode 100644 plugins/example/example.c
> 
> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> index adf5ccb..18624aa 100644
> --- a/Documentation/manual.txt
> +++ b/Documentation/manual.txt
> @@ -10,6 +10,10 @@ I - "dtc", the device tree compiler
>      4.1) Overview
>      4.2) Properties
>      4.3) Labels and References
> +    5) Plugins
> +    5.1) Loading plugins
> +    5.2) Exporting Functionality
> +    5.3) Building Plugins
>  
>  II - The DT block format
>      1) Header
> @@ -115,6 +119,16 @@ Options:
>      -d <dependency_filename>
>  	Generate a dependency file during compilation.
>  
> +    -l, --plugin <plugin name>[,key][,value]
> +	Load a plugin and, optionally, pass it an argument.
> +		plugin name - the name of the shared object without the extension (can contain a path)
> +		key         - the name of the argument
> +		value       - can be omitted
> +	Example: dtc -lsome-plugin,o,out.dts -lsome-plugin,help -l path/to/another-plugin [...]
> +
> +    -L, --plugin-dir <arg>
> +	The directory in which to search for plugins
> +
>      -q
>  	Quiet: -q suppress warnings, -qq errors, -qqq all
>  
> @@ -272,6 +286,66 @@ And used in properties, labels may appear before or after any value:
>      };
>  
>  
> +5) Plugins
> +
> +Plugins are shared libraries that DTC loads at runtime using
> +
> +	dlopen(path, RTLD_NOW | RTLD_GLOBAL | RTLD_DEEPBIND)
> +
> +RTLD_NOW means they are loaded immediately (right before the parsing
> +stage).
> +
> +RTLD_GLOBAL means their symbols can be used by other plugins.
> +
> +RTLD_DEEPBIND means the dynamic linker will look for symbols within the
> +plugin first, before searching in the main program and other plugins.
> +
> +All plugins must include the "dtc-plugin.h" header.
> +
> +
> +5.1) Loading plugins
> +
> +Please, refer to the -l and -L options in the "Command Line" section.
> +
> +
> +5.2) Exporting Functionality
> +
> +Plugins export functionality through the "EXPORT_FUNCTION(type, fn)"
> +macro, where "type" is one of the typedefs from "dtc-plugin.h", and
> +"fn" is the function that will be called by DTC. Each type of function
> +gets called once for each plugin.
> +
> +Every plugin must export a function of type "init_fn_t".
> +
> +"plugins/example/example.c" shows how an init function can be used.
> +
> +
> +5.3) Building Plugins
> +
> +The command "make plugins" infers the names of the shared library files
> +from the names of directories under "plugins/". For example, the
> +presence of a
> +
> +	plugins/example/
> +
> +directory means that make will try to build
> +
> +	plugins/example/example.so
> +
> +from
> +
> +	plugins/example/example.c
> +
> +It will then make a symlink
> +
> +	plugins/example.so
> +
> +for convenience.
> +
> +Please, see "plugins/example/Makefile.example" for how to override the
> +defaut behavior.
> +
> +
>  
>  II - The DT block format
>  ========================
> diff --git a/plugins/example/Makefile.example b/plugins/example/Makefile.example
> new file mode 100644
> index 0000000..56d4e17
> --- /dev/null
> +++ b/plugins/example/Makefile.example
> @@ -0,0 +1,19 @@
> +# Optional per-plugin makefile
> +#
> +# Here you can add gcc flags:
> +# PLUGIN_CFLAGS_example = -some-flag
> +#
> +# Add libraries:
> +# PLUGIN_LDLIBS_example = -lsome-lib
> +#
> +# Add files to clean:
> +# PLUGIN_CLEANFILES += $(PLUGIN_dir)/example/*.tmp
> +#
> +# Or override the default rules:
> +# $(PLUGIN_dir)/example/example.$(SHAREDLIB_EXT): $(PLUGIN_dir)/example/example.o
> +#	@$(VECHO) LD $@
> +#	...
> +#
> +# $(PLUGIN_dir)/example/example.o: $(PLUGIN_dir)/example/example.c
> +#	@$(VECHO) CC $@
> +#	...
> diff --git a/plugins/example/example.c b/plugins/example/example.c
> new file mode 100644
> index 0000000..3bbf9ea
> --- /dev/null
> +++ b/plugins/example/example.c
> @@ -0,0 +1,29 @@
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "dtc-plugin.h"
> +
> +#define NAME "example: "
> +
> +static int init_example(struct plugin_version v, int argc, struct plugin_arg *argv)
> +{
> +	if (!dtc_plugin_default_version_check(v)) {
> +		fprintf(stderr, NAME"Plugin is incompatible with this version of DTC\n");
> +		return 1;
> +	}
> +
> +	for (int i = 0; i < argc; i++) {
> +		if (strcmp(argv[i].key, "h") == 0
> +		 || strcmp(argv[i].key, "help") == 0) {
> +			printf(NAME"This is an example plugin. It prints its arguments.\n");
> +			return 1;
> +		} else {
> +			printf(NAME"%s: %s\n", argv[i].key,
> +			       argv[i].value ? argv[i].value : "");
> +		}
> +	}
> +
> +	printf(NAME"Loaded plugin 'example'\n");
> +	return 0;
> +}
> +EXPORT_FUNCTION(init_fn_t, init_example);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 2/4] dtc: Add plugin documentation and examples

[Andrei Ziureaev]

On 8/3/20 9:08 AM, David Gibson wrote:
> On Tue, Jul 21, 2020 at 04:58:58PM +0100, Andrei Ziureaev wrote:
>> Document the plugin API in Documentation/manual.txt and provide an
>> example plugin.
>>
>> Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
> So, thanks for starting out with some docs.  However, I don't think
> these docs cover the most important things for them to cover: what the
> API is for the plugin - what information goes in, what information
> goes out, and what all the supported entry points are.  Understanding
> the data model of the plugin would make reviewing the rest of the
> details much easier.
OK, so I'll add some documentation to dtc-plugin.h and duplicate that in
the manual. That will look something like this:

/*
  * Function types that plugins can export.
  * DTC calls each type exactly once for each plugin.
  */

/**
  * Called right after the plugin is loaded.
  *
  * Every plugin must export a function of this type.
  *
  * @param dtc_ver    DTC's plugin API version
  * @param argc       Length of argv
  * @param argv       Array of plugin arguments
  * @return 0 on success, or 1 on failure
  */
typedef int (*init_fn_t)(struct plugin_version dtc_ver, int argc,
                          struct plugin_arg *argv);

/**
  * Called after DTC's parsing stage, but before the output stage.
  *
  * @param dti    DTC's internal live tree. Implementations can modify
  *               the live tree and "pass it back" to DTC and to
  *               subsequent plugins.
  */
typedef void (*validate_fn_t)(struct dt_info *dti);

One other point: we could pass validate_fn_t a copy of the live tree.
Then plugins won't be able to "pass back" the live tree. The external
live tree could then be a completely separate struct from the internal
one. So, less flexibility for plugins, but a better, more stable API.
>
>> ---
>>   Documentation/manual.txt         | 74 ++++++++++++++++++++++++++++++++
>>   plugins/example/Makefile.example | 19 ++++++++
>>   plugins/example/example.c        | 29 +++++++++++++
>>   3 files changed, 122 insertions(+)
>>   create mode 100644 plugins/example/Makefile.example
>>   create mode 100644 plugins/example/example.c
>>
>> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
>> index adf5ccb..18624aa 100644
>> --- a/Documentation/manual.txt
>> +++ b/Documentation/manual.txt
>> @@ -10,6 +10,10 @@ I - "dtc", the device tree compiler
>>       4.1) Overview
>>       4.2) Properties
>>       4.3) Labels and References
>> +    5) Plugins
>> +    5.1) Loading plugins
>> +    5.2) Exporting Functionality
>> +    5.3) Building Plugins
>>
>>   II - The DT block format
>>       1) Header
>> @@ -115,6 +119,16 @@ Options:
>>       -d <dependency_filename>
>>      Generate a dependency file during compilation.
>>
>> +    -l, --plugin <plugin name>[,key][,value]
>> +    Load a plugin and, optionally, pass it an argument.
>> +            plugin name - the name of the shared object without the extension (can contain a path)
>> +            key         - the name of the argument
>> +            value       - can be omitted
>> +    Example: dtc -lsome-plugin,o,out.dts -lsome-plugin,help -l path/to/another-plugin [...]
>> +
>> +    -L, --plugin-dir <arg>
>> +    The directory in which to search for plugins
>> +
>>       -q
>>      Quiet: -q suppress warnings, -qq errors, -qqq all
>>
>> @@ -272,6 +286,66 @@ And used in properties, labels may appear before or after any value:
>>       };
>>
>>
>> +5) Plugins
>> +
>> +Plugins are shared libraries that DTC loads at runtime using
>> +
>> +    dlopen(path, RTLD_NOW | RTLD_GLOBAL | RTLD_DEEPBIND)
>> +
>> +RTLD_NOW means they are loaded immediately (right before the parsing
>> +stage).
>> +
>> +RTLD_GLOBAL means their symbols can be used by other plugins.
>> +
>> +RTLD_DEEPBIND means the dynamic linker will look for symbols within the
>> +plugin first, before searching in the main program and other plugins.
>> +
>> +All plugins must include the "dtc-plugin.h" header.
>> +
>> +
>> +5.1) Loading plugins
>> +
>> +Please, refer to the -l and -L options in the "Command Line" section.
>> +
>> +
>> +5.2) Exporting Functionality
>> +
>> +Plugins export functionality through the "EXPORT_FUNCTION(type, fn)"
>> +macro, where "type" is one of the typedefs from "dtc-plugin.h", and
>> +"fn" is the function that will be called by DTC. Each type of function
>> +gets called once for each plugin.
>> +
>> +Every plugin must export a function of type "init_fn_t".
>> +
>> +"plugins/example/example.c" shows how an init function can be used.
>> +
>> +
>> +5.3) Building Plugins
>> +
>> +The command "make plugins" infers the names of the shared library files
>> +from the names of directories under "plugins/". For example, the
>> +presence of a
>> +
>> +    plugins/example/
>> +
>> +directory means that make will try to build
>> +
>> +    plugins/example/example.so
>> +
>> +from
>> +
>> +    plugins/example/example.c
>> +
>> +It will then make a symlink
>> +
>> +    plugins/example.so
>> +
>> +for convenience.
>> +
>> +Please, see "plugins/example/Makefile.example" for how to override the
>> +defaut behavior.
>> +
>> +
>>
>>   II - The DT block format
>>   ========================
>> diff --git a/plugins/example/Makefile.example b/plugins/example/Makefile.example
>> new file mode 100644
>> index 0000000..56d4e17
>> --- /dev/null
>> +++ b/plugins/example/Makefile.example
>> @@ -0,0 +1,19 @@
>> +# Optional per-plugin makefile
>> +#
>> +# Here you can add gcc flags:
>> +# PLUGIN_CFLAGS_example = -some-flag
>> +#
>> +# Add libraries:
>> +# PLUGIN_LDLIBS_example = -lsome-lib
>> +#
>> +# Add files to clean:
>> +# PLUGIN_CLEANFILES += $(PLUGIN_dir)/example/*.tmp
>> +#
>> +# Or override the default rules:
>> +# $(PLUGIN_dir)/example/example.$(SHAREDLIB_EXT): $(PLUGIN_dir)/example/example.o
>> +#   @$(VECHO) LD $@
>> +#   ...
>> +#
>> +# $(PLUGIN_dir)/example/example.o: $(PLUGIN_dir)/example/example.c
>> +#   @$(VECHO) CC $@
>> +#   ...
>> diff --git a/plugins/example/example.c b/plugins/example/example.c
>> new file mode 100644
>> index 0000000..3bbf9ea
>> --- /dev/null
>> +++ b/plugins/example/example.c
>> @@ -0,0 +1,29 @@
>> +#include <stdio.h>
>> +#include <string.h>
>> +
>> +#include "dtc-plugin.h"
>> +
>> +#define NAME "example: "
>> +
>> +static int init_example(struct plugin_version v, int argc, struct plugin_arg *argv)
>> +{
>> +    if (!dtc_plugin_default_version_check(v)) {
>> +            fprintf(stderr, NAME"Plugin is incompatible with this version of DTC\n");
>> +            return 1;
>> +    }
>> +
>> +    for (int i = 0; i < argc; i++) {
>> +            if (strcmp(argv[i].key, "h") == 0
>> +             || strcmp(argv[i].key, "help") == 0) {
>> +                    printf(NAME"This is an example plugin. It prints its arguments.\n");
>> +                    return 1;
>> +            } else {
>> +                    printf(NAME"%s: %s\n", argv[i].key,
>> +                           argv[i].value ? argv[i].value : "");
>> +            }
>> +    }
>> +
>> +    printf(NAME"Loaded plugin 'example'\n");
>> +    return 0;
>> +}
>> +EXPORT_FUNCTION(init_fn_t, init_example);
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [RFC PATCH 2/4] dtc: Add plugin documentation and examples

[Andrei Ziureaev]

On 8/3/20 3:09 PM, Andrei Ziureaev wrote:

 > IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended 
recipient, please notify the sender immediately and do not disclose the 
contents to any other person, use it for any purpose, or store or copy 
the information in any medium. Thank you.

Oops. Please, ignore this.

[RFC PATCH 3/4] dtc: Move some definitions into dtc-plugin.h

[Andrei Ziureaev]

Put various bits and pieces into the header for plugins.

Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
---
 dtc-plugin.h | 173 +++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h        | 145 +-----------------------------------------
 treesource.c |  21 -------
 3 files changed, 174 insertions(+), 165 deletions(-)
 create mode 100644 dtc-plugin.h

diff --git a/dtc-plugin.h b/dtc-plugin.h
new file mode 100644
index 0000000..35c3d19
--- /dev/null
+++ b/dtc-plugin.h
@@ -0,0 +1,173 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef DTC_PLUGIN_H
+#define DTC_PLUGIN_H
+
+/*
+ * (C) Copyright Arm Holdings.  2020
+ * (C) Copyright David Gibson <dwg-8fk3Idey6ehBDgjK7y7TUQ@public.gmane.org>, IBM Corporation.  2005.
+ */
+
+#include <stdint.h>
+#include <stdbool.h>
+
+struct dt_info {
+	const char *version;
+	unsigned int dtsflags;
+	struct reserve_info *reservelist;
+	uint32_t boot_cpuid_phys;
+	struct node *dt;		/* the device tree */
+	const char *outname;		/* filename being written to, "-" for stdout */
+};
+
+typedef uint32_t cell_t;
+
+enum markertype {
+	TYPE_NONE,
+	REF_PHANDLE,
+	REF_PATH,
+	LABEL,
+	TYPE_UINT8,
+	TYPE_UINT16,
+	TYPE_UINT32,
+	TYPE_UINT64,
+	TYPE_STRING,
+};
+
+struct marker {
+	enum markertype type;
+	int offset;
+	char *ref;
+	struct marker *next;
+};
+
+struct data {
+	int len;
+	char *val;
+	struct marker *markers;
+};
+
+#define for_each_marker(m) \
+	for (; (m); (m) = (m)->next)
+#define for_each_marker_of_type(m, t) \
+	for_each_marker(m) \
+		if ((m)->type == (t))
+
+/* Live trees */
+struct label {
+	bool deleted;
+	char *label;
+	struct label *next;
+};
+
+struct bus_type {
+	const char *name;
+};
+
+struct property {
+	bool deleted;
+	char *name;
+	struct data val;
+
+	struct property *next;
+
+	struct label *labels;
+	struct srcpos *srcpos;
+};
+
+struct node {
+	bool deleted;
+	char *name;
+	struct property *proplist;
+	struct node *children;
+
+	struct node *parent;
+	struct node *next_sibling;
+
+	char *fullpath;
+	int basenamelen;
+
+	cell_t phandle;
+	int addr_cells, size_cells;
+
+	struct label *labels;
+	const struct bus_type *bus;
+	struct srcpos *srcpos;
+
+	bool omit_if_unused, is_referenced;
+};
+
+#define for_each_label_withdel(l0, l) \
+	for ((l) = (l0); (l); (l) = (l)->next)
+
+#define for_each_label(l0, l) \
+	for_each_label_withdel(l0, l) \
+		if (!(l)->deleted)
+
+#define for_each_property_withdel(n, p) \
+	for ((p) = (n)->proplist; (p); (p) = (p)->next)
+
+#define for_each_property(n, p) \
+	for_each_property_withdel(n, p) \
+		if (!(p)->deleted)
+
+#define for_each_child_withdel(n, c) \
+	for ((c) = (n)->children; (c); (c) = (c)->next_sibling)
+
+#define for_each_child(n, c) \
+	for_each_child_withdel(n, c) \
+		if (!(c)->deleted)
+
+static inline uint16_t dtb_ld16(const void *p)
+{
+	const uint8_t *bp = (const uint8_t *)p;
+
+	return ((uint16_t)bp[0] << 8)
+		| bp[1];
+}
+
+static inline uint32_t dtb_ld32(const void *p)
+{
+	const uint8_t *bp = (const uint8_t *)p;
+
+	return ((uint32_t)bp[0] << 24)
+		| ((uint32_t)bp[1] << 16)
+		| ((uint32_t)bp[2] << 8)
+		| bp[3];
+}
+
+static inline uint64_t dtb_ld64(const void *p)
+{
+	const uint8_t *bp = (const uint8_t *)p;
+
+	return ((uint64_t)bp[0] << 56)
+		| ((uint64_t)bp[1] << 48)
+		| ((uint64_t)bp[2] << 40)
+		| ((uint64_t)bp[3] << 32)
+		| ((uint64_t)bp[4] << 24)
+		| ((uint64_t)bp[5] << 16)
+		| ((uint64_t)bp[6] << 8)
+		| bp[7];
+}
+
+static inline bool has_data_type_information(struct marker *m)
+{
+	return m->type >= TYPE_UINT8;
+}
+
+static inline struct marker *next_type_marker(struct marker *m)
+{
+	while (m && !has_data_type_information(m))
+		m = m->next;
+	return m;
+}
+
+static inline size_t type_marker_length(struct marker *m)
+{
+	struct marker *next = next_type_marker(m->next);
+
+	if (next)
+		return next->offset - m->offset;
+	return 0;
+}
+
+#endif /* DTC_PLUGIN_H */
diff --git a/dtc.h b/dtc.h
index a08f415..f31aed7 100644
--- a/dtc.h
+++ b/dtc.h
@@ -22,6 +22,7 @@
 #include <fdt.h>
 
 #include "util.h"
+#include "dtc-plugin.h"
 
 #ifdef DEBUG
 #define debug(...)	printf(__VA_ARGS__)
@@ -49,84 +50,14 @@ extern int annotate;		/* annotate .dts with input source location */
 #define PHANDLE_EPAPR	0x2
 #define PHANDLE_BOTH	0x3
 
-typedef uint32_t cell_t;
-
-static inline uint16_t dtb_ld16(const void *p)
-{
-	const uint8_t *bp = (const uint8_t *)p;
-
-	return ((uint16_t)bp[0] << 8)
-		| bp[1];
-}
-
-static inline uint32_t dtb_ld32(const void *p)
-{
-	const uint8_t *bp = (const uint8_t *)p;
-
-	return ((uint32_t)bp[0] << 24)
-		| ((uint32_t)bp[1] << 16)
-		| ((uint32_t)bp[2] << 8)
-		| bp[3];
-}
-
-static inline uint64_t dtb_ld64(const void *p)
-{
-	const uint8_t *bp = (const uint8_t *)p;
-
-	return ((uint64_t)bp[0] << 56)
-		| ((uint64_t)bp[1] << 48)
-		| ((uint64_t)bp[2] << 40)
-		| ((uint64_t)bp[3] << 32)
-		| ((uint64_t)bp[4] << 24)
-		| ((uint64_t)bp[5] << 16)
-		| ((uint64_t)bp[6] << 8)
-		| bp[7];
-}
-
 #define streq(a, b)	(strcmp((a), (b)) == 0)
 #define strstarts(s, prefix)	(strncmp((s), (prefix), strlen(prefix)) == 0)
 #define strprefixeq(a, n, b)	(strlen(b) == (n) && (memcmp(a, b, n) == 0))
 
 #define ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
 
-/* Data blobs */
-enum markertype {
-	TYPE_NONE,
-	REF_PHANDLE,
-	REF_PATH,
-	LABEL,
-	TYPE_UINT8,
-	TYPE_UINT16,
-	TYPE_UINT32,
-	TYPE_UINT64,
-	TYPE_STRING,
-};
-extern const char *markername(enum markertype markertype);
-
-struct  marker {
-	enum markertype type;
-	int offset;
-	char *ref;
-	struct marker *next;
-};
-
-struct data {
-	int len;
-	char *val;
-	struct marker *markers;
-};
-
-
 #define empty_data ((struct data){ 0 /* all .members = 0 or NULL */ })
 
-#define for_each_marker(m) \
-	for (; (m); (m) = (m)->next)
-#define for_each_marker_of_type(m, t) \
-	for_each_marker(m) \
-		if ((m)->type == (t))
-
-size_t type_marker_length(struct marker *m);
-
 void data_free(struct data d);
 
 struct data data_grow_for(struct data d, int xlen);
@@ -156,71 +87,6 @@ bool data_is_one_string(struct data d);
 #define MAX_PROPNAME_LEN	31
 #define MAX_NODENAME_LEN	31
 
-/* Live trees */
-struct label {
-	bool deleted;
-	char *label;
-	struct label *next;
-};
-
-struct bus_type {
-	const char *name;
-};
-
-struct property {
-	bool deleted;
-	char *name;
-	struct data val;
-
-	struct property *next;
-
-	struct label *labels;
-	struct srcpos *srcpos;
-};
-
-struct node {
-	bool deleted;
-	char *name;
-	struct property *proplist;
-	struct node *children;
-
-	struct node *parent;
-	struct node *next_sibling;
-
-	char *fullpath;
-	int basenamelen;
-
-	cell_t phandle;
-	int addr_cells, size_cells;
-
-	struct label *labels;
-	const struct bus_type *bus;
-	struct srcpos *srcpos;
-
-	bool omit_if_unused, is_referenced;
-};
-
-#define for_each_label_withdel(l0, l) \
-	for ((l) = (l0); (l); (l) = (l)->next)
-
-#define for_each_label(l0, l) \
-	for_each_label_withdel(l0, l) \
-		if (!(l)->deleted)
-
-#define for_each_property_withdel(n, p) \
-	for ((p) = (n)->proplist; (p); (p) = (p)->next)
-
-#define for_each_property(n, p) \
-	for_each_property_withdel(n, p) \
-		if (!(p)->deleted)
-
-#define for_each_child_withdel(n, c) \
-	for ((c) = (n)->children; (c); (c) = (c)->next_sibling)
-
-#define for_each_child(n, c) \
-	for_each_child_withdel(n, c) \
-		if (!(c)->deleted)
-
 void add_label(struct label **labels, char *label);
 void delete_labels(struct label **labels);
 
@@ -283,15 +149,6 @@ struct reserve_info *chain_reserve_entry(struct reserve_info *first,
 struct reserve_info *add_reserve_entry(struct reserve_info *list,
 				       struct reserve_info *new);
 
-
-struct dt_info {
-	unsigned int dtsflags;
-	struct reserve_info *reservelist;
-	uint32_t boot_cpuid_phys;
-	struct node *dt;		/* the device tree */
-	const char *outname;		/* filename being written to, "-" for stdout */
-};
-
 /* DTS version flags definitions */
 #define DTSF_V1		0x0001	/* /dts-v1/ */
 #define DTSF_PLUGIN	0x0002	/* /plugin/ */
diff --git a/treesource.c b/treesource.c
index 061ba8c..03aad68 100644
--- a/treesource.c
+++ b/treesource.c
@@ -124,27 +124,6 @@ static void write_propval_int(FILE *f, const char *p, size_t len, size_t width)
 	}
 }
 
-static bool has_data_type_information(struct marker *m)
-{
-	return m->type >= TYPE_UINT8;
-}
-
-static struct marker *next_type_marker(struct marker *m)
-{
-	while (m && !has_data_type_information(m))
-		m = m->next;
-	return m;
-}
-
-size_t type_marker_length(struct marker *m)
-{
-	struct marker *next = next_type_marker(m->next);
-
-	if (next)
-		return next->offset - m->offset;
-	return 0;
-}
-
 static const char *delim_start[] = {
 	[TYPE_UINT8] = "[",
 	[TYPE_UINT16] = "/bits/ 16 <",
-- 
2.17.1

Re: [RFC PATCH 3/4] dtc: Move some definitions into dtc-plugin.h

[Rob Herring]

On Tue, Jul 21, 2020 at 9:59 AM Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org> wrote:
>
> Put various bits and pieces into the header for plugins.
>
> Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
> ---
>  dtc-plugin.h | 173 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  dtc.h        | 145 +-----------------------------------------
>  treesource.c |  21 -------
>  3 files changed, 174 insertions(+), 165 deletions(-)
>  create mode 100644 dtc-plugin.h
>
> diff --git a/dtc-plugin.h b/dtc-plugin.h
> new file mode 100644
> index 0000000..35c3d19
> --- /dev/null
> +++ b/dtc-plugin.h
> @@ -0,0 +1,173 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef DTC_PLUGIN_H
> +#define DTC_PLUGIN_H
> +
> +/*
> + * (C) Copyright Arm Holdings.  2020
> + * (C) Copyright David Gibson <dwg-8fk3Idey6ehBDgjK7y7TUQ@public.gmane.org>, IBM Corporation.  2005.
> + */
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +
> +struct dt_info {
> +       const char *version;

I think just an int is sufficient here.

> +       unsigned int dtsflags;
> +       struct reserve_info *reservelist;

This struct needs to be defined in this header.

> +       uint32_t boot_cpuid_phys;
> +       struct node *dt;                /* the device tree */
> +       const char *outname;            /* filename being written to, "-" for stdout */
> +};
> +
> +typedef uint32_t cell_t;
> +
> +enum markertype {
> +       TYPE_NONE,
> +       REF_PHANDLE,
> +       REF_PATH,
> +       LABEL,
> +       TYPE_UINT8,
> +       TYPE_UINT16,
> +       TYPE_UINT32,
> +       TYPE_UINT64,
> +       TYPE_STRING,
> +};
> +
> +struct marker {
> +       enum markertype type;
> +       int offset;
> +       char *ref;
> +       struct marker *next;
> +};
> +
> +struct data {
> +       int len;
> +       char *val;
> +       struct marker *markers;
> +};
> +
> +#define for_each_marker(m) \
> +       for (; (m); (m) = (m)->next)
> +#define for_each_marker_of_type(m, t) \
> +       for_each_marker(m) \
> +               if ((m)->type == (t))
> +
> +/* Live trees */
> +struct label {
> +       bool deleted;
> +       char *label;
> +       struct label *next;
> +};
> +
> +struct bus_type {
> +       const char *name;
> +};
> +
> +struct property {
> +       bool deleted;
> +       char *name;
> +       struct data val;
> +
> +       struct property *next;
> +
> +       struct label *labels;
> +       struct srcpos *srcpos;

This struct also needs to be defined in here.

> +};
> +
> +struct node {
> +       bool deleted;
> +       char *name;
> +       struct property *proplist;
> +       struct node *children;
> +
> +       struct node *parent;
> +       struct node *next_sibling;
> +
> +       char *fullpath;
> +       int basenamelen;
> +
> +       cell_t phandle;
> +       int addr_cells, size_cells;
> +
> +       struct label *labels;
> +       const struct bus_type *bus;
> +       struct srcpos *srcpos;
> +
> +       bool omit_if_unused, is_referenced;
> +};
> +
> +#define for_each_label_withdel(l0, l) \
> +       for ((l) = (l0); (l); (l) = (l)->next)
> +
> +#define for_each_label(l0, l) \
> +       for_each_label_withdel(l0, l) \
> +               if (!(l)->deleted)
> +
> +#define for_each_property_withdel(n, p) \
> +       for ((p) = (n)->proplist; (p); (p) = (p)->next)
> +
> +#define for_each_property(n, p) \
> +       for_each_property_withdel(n, p) \
> +               if (!(p)->deleted)
> +
> +#define for_each_child_withdel(n, c) \
> +       for ((c) = (n)->children; (c); (c) = (c)->next_sibling)
> +
> +#define for_each_child(n, c) \
> +       for_each_child_withdel(n, c) \
> +               if (!(c)->deleted)
> +
> +static inline uint16_t dtb_ld16(const void *p)
> +{
> +       const uint8_t *bp = (const uint8_t *)p;
> +
> +       return ((uint16_t)bp[0] << 8)
> +               | bp[1];
> +}
> +
> +static inline uint32_t dtb_ld32(const void *p)
> +{
> +       const uint8_t *bp = (const uint8_t *)p;
> +
> +       return ((uint32_t)bp[0] << 24)
> +               | ((uint32_t)bp[1] << 16)
> +               | ((uint32_t)bp[2] << 8)
> +               | bp[3];
> +}
> +
> +static inline uint64_t dtb_ld64(const void *p)
> +{
> +       const uint8_t *bp = (const uint8_t *)p;
> +
> +       return ((uint64_t)bp[0] << 56)
> +               | ((uint64_t)bp[1] << 48)
> +               | ((uint64_t)bp[2] << 40)
> +               | ((uint64_t)bp[3] << 32)
> +               | ((uint64_t)bp[4] << 24)
> +               | ((uint64_t)bp[5] << 16)
> +               | ((uint64_t)bp[6] << 8)
> +               | bp[7];
> +}
> +
> +static inline bool has_data_type_information(struct marker *m)
> +{
> +       return m->type >= TYPE_UINT8;
> +}
> +
> +static inline struct marker *next_type_marker(struct marker *m)
> +{
> +       while (m && !has_data_type_information(m))
> +               m = m->next;
> +       return m;
> +}
> +
> +static inline size_t type_marker_length(struct marker *m)
> +{
> +       struct marker *next = next_type_marker(m->next);
> +
> +       if (next)
> +               return next->offset - m->offset;
> +       return 0;
> +}
> +
> +#endif /* DTC_PLUGIN_H */

[RFC PATCH 4/4] dtc: Add a plugin interface

[Andrei Ziureaev]

Add support for building and loading plugins.

A plugin interface makes it possible to add checks in any language. It
also allows these checks to print dts source line information.

Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
---
 Makefile     |  32 +++++++++++-
 dtc-plugin.h |  27 ++++++++++
 dtc.c        | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 dtc.h        |  47 +++++++++++++++++
 4 files changed, 243 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index cb256e8..76bba53 100644
--- a/Makefile
+++ b/Makefile
@@ -25,6 +25,8 @@ WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
 	-Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
 CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
 
+LDLIBS_dtc += -ldl -export-dynamic
+
 BISON = bison
 LEX = flex
 SWIG = swig
@@ -65,14 +67,17 @@ ifeq ($(HOSTOS),darwin)
 SHAREDLIB_EXT     = dylib
 SHAREDLIB_CFLAGS  = -fPIC
 SHAREDLIB_LDFLAGS = -fPIC -dynamiclib -Wl,-install_name -Wl,
+PLUGIN_ldflags = -fPIC -dynamiclib
 else ifeq ($(HOSTOS),$(filter $(HOSTOS),msys cygwin))
 SHAREDLIB_EXT     = so
 SHAREDLIB_CFLAGS  =
 SHAREDLIB_LDFLAGS = -shared -Wl,--version-script=$(LIBFDT_version) -Wl,-soname,
+PLUGIN_ldflags = -shared
 else
 SHAREDLIB_EXT     = so
 SHAREDLIB_CFLAGS  = -fPIC
 SHAREDLIB_LDFLAGS = -fPIC -shared -Wl,--version-script=$(LIBFDT_version) -Wl,-soname,
+PLUGIN_ldflags = -fPIC -shared
 endif
 
 #
@@ -186,7 +191,32 @@ ifneq ($(MAKECMDGOALS),libfdt)
 endif
 endif
 
+#
+# Rules for plugins
+#
+PLUGIN_dir = plugins
+PLUGIN_subdirs = $(patsubst %/,%,$(filter-out $(PLUGIN_dir)/,$(dir $(wildcard $(PLUGIN_dir)/*/))))
+PLUGIN_names = $(notdir $(PLUGIN_subdirs))
+PLUGIN_libs = $(join $(PLUGIN_subdirs),$(patsubst %,/%.$(SHAREDLIB_EXT),$(PLUGIN_names)))
+PLUGIN_CLEANFILES += $(PLUGIN_dir)/*.$(SHAREDLIB_EXT)
+PLUGIN_CLEANFILES += $(addprefix $(PLUGIN_dir)/*/,*.o *.$(SHAREDLIB_EXT))
+
+include $(wildcard $(PLUGIN_dir)/*/Makefile.*)
+
+plugins: $(PLUGIN_libs)
+
+$(PLUGIN_dir)/%.$(SHAREDLIB_EXT): $(PLUGIN_dir)/%.o
+	@$(VECHO) LD $@
+	$(LINK.c) $(PLUGIN_ldflags) -o $@ $< $(PLUGIN_LDLIBS_$(notdir $*))
+	ln -sf $(patsubst $(PLUGIN_dir)/%,%,$@) $(PLUGIN_dir)/$(notdir $@)
+
+$(PLUGIN_dir)/%.o: $(PLUGIN_dir)/%.c
+	@$(VECHO) CC $@
+	$(CC) $(CPPFLAGS) $(CFLAGS) $(PLUGIN_CFLAGS_$(notdir $*)) -o $@ -c $<
 
+plugins_clean:
+	@$(VECHO) CLEAN "(plugins)"
+	rm -f $(PLUGIN_CLEANFILES)
 
 #
 # Rules for libfdt
@@ -330,7 +360,7 @@ endif
 STD_CLEANFILES = *~ *.o *.$(SHAREDLIB_EXT) *.d *.a *.i *.s core a.out vgcore.* \
 	*.tab.[ch] *.lex.c *.output
 
-clean: libfdt_clean pylibfdt_clean tests_clean
+clean: libfdt_clean pylibfdt_clean tests_clean plugins_clean
 	@$(VECHO) CLEAN
 	rm -f $(STD_CLEANFILES)
 	rm -f $(VERSION_FILE)
diff --git a/dtc-plugin.h b/dtc-plugin.h
index 35c3d19..9ec6b81 100644
--- a/dtc-plugin.h
+++ b/dtc-plugin.h
@@ -10,6 +10,8 @@
 #include <stdint.h>
 #include <stdbool.h>
 
+#include "srcpos.h"
+
 struct dt_info {
 	const char *version;
 	unsigned int dtsflags;
@@ -170,4 +172,29 @@ static inline size_t type_marker_length(struct marker *m)
 	return 0;
 }
 
+struct plugin_arg {
+	char *key;	/* A non-empty string */
+	char *value;	/* NULL or a non-empty string */
+};
+
+struct plugin_version {
+	int major;	/* Incompatible changes */
+	int minor;	/* Somewhat incompatible changes */
+};
+
+#define DTC_PLUGIN_API_VERSION ((struct plugin_version){ .major = 0, .minor = 0 })
+
+static inline bool dtc_plugin_default_version_check(struct plugin_version v)
+{
+	struct plugin_version this_v = DTC_PLUGIN_API_VERSION;
+	return v.major == this_v.major && v.minor == this_v.minor;
+}
+
+/* Hook definitions */
+typedef int (*init_fn_t)(struct plugin_version v, int argc, struct plugin_arg *argv);
+typedef void (*validate_fn_t)(struct dt_info *dti);
+
+#define TYPE_TO_NAME(type) dtc_plugin_v0_##type
+#define EXPORT_FUNCTION(type, fn) type TYPE_TO_NAME(type) = (fn)
+
 #endif /* DTC_PLUGIN_H */
diff --git a/dtc.c b/dtc.c
index bdb3f59..5a9b118 100644
--- a/dtc.c
+++ b/dtc.c
@@ -4,6 +4,7 @@
  */
 
 #include <sys/stat.h>
+#include <dlfcn.h>
 
 #include "dtc.h"
 #include "srcpos.h"
@@ -47,7 +48,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
 
 /* Usage related data. */
 static const char usage_synopsis[] = "dtc [options] <input file>";
-static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AThv";
+static const char usage_short_opts[] = "qI:O:o:V:d:l:L:R:S:p:a:fb:i:H:sW:E:@AThv";
 static struct option const usage_long_opts[] = {
 	{"quiet",            no_argument, NULL, 'q'},
 	{"in-format",         a_argument, NULL, 'I'},
@@ -55,6 +56,8 @@ static struct option const usage_long_opts[] = {
 	{"out-format",        a_argument, NULL, 'O'},
 	{"out-version",       a_argument, NULL, 'V'},
 	{"out-dependency",    a_argument, NULL, 'd'},
+	{"plugin",            a_argument, NULL, 'l'},
+	{"plugin-dir",        a_argument, NULL, 'L'},
 	{"reserve",           a_argument, NULL, 'R'},
 	{"space",             a_argument, NULL, 'S'},
 	{"pad",               a_argument, NULL, 'p'},
@@ -89,6 +92,13 @@ static const char * const usage_opts_help[] = {
 	 "\t\tasm - assembler source",
 	"\n\tBlob version to produce, defaults to "stringify(DEFAULT_FDT_VERSION)" (for dtb and asm output)",
 	"\n\tOutput dependency file",
+	"\n\tLoad a plugin and, optionally, pass it an argument.\n"
+	"\tUsage: -l <plugin name>[,key][,value]\n"
+	 "\t\tplugin name - the name of the shared object without the extension (can contain a path)\n"
+	 "\t\tkey         - the name of the argument\n"
+	 "\t\tvalue       - can be omitted\n"
+	"\tExample: dtc -lsome-plugin,o,out.dts -lsome-plugin,help -l path/to/another-plugin [...]",
+	"\n\tThe directory in which to search for plugins",
 	"\n\tMake space for <number> reserve map entries (for dtb and asm output)",
 	"\n\tMake the blob at least <bytes> long (extra space)",
 	"\n\tAdd padding to the blob of <bytes> long (extra space)",
@@ -157,6 +167,114 @@ static const char *guess_input_format(const char *fname, const char *fallback)
 	return guess_type_by_name(fname, fallback);
 }
 
+static struct plugin *get_plugin_with_path(struct plugin_array *plugins,
+					   char *path)
+{
+	struct plugin *p;
+	array_for_each(plugins, p)
+		if (strcmp(p->path, path) == 0)
+			return p;
+
+	return NULL;
+}
+
+static void parse_plugin_info(char *arg, const char *plugin_dir, char **path,
+			      char **key, char **value)
+{
+	char *name;
+	size_t dirlen;
+	char *tmp;
+	size_t tmplen;
+
+	if (arg == NULL || *arg == '\0' || *arg == ',')
+		return;
+
+	name = strtok(arg, ",");
+
+	dirlen = strlen(plugin_dir);
+	tmplen = dirlen + strlen(name) + strlen(SHAREDLIB_EXT) + 2;
+	tmp = xmalloc(tmplen);
+
+	if (plugin_dir[0] == '\0' || plugin_dir[dirlen - 1] == DIR_SEPARATOR)
+		snprintf(tmp, tmplen, "%s%s%s", plugin_dir,
+			 name, SHAREDLIB_EXT);
+	else
+		snprintf(tmp, tmplen, "%s%c%s%s", plugin_dir,
+			 DIR_SEPARATOR, name, SHAREDLIB_EXT);
+
+	*path = realpath(tmp, NULL); /* malloc path */
+	if (*path == NULL)
+		die("Couldn't resolve plugin path %s: %s\n", tmp, strerror(errno));
+
+	*key = strtok(NULL, ",");
+	*value = strtok(NULL, "");
+	free(tmp);
+}
+
+static void register_plugin_info(struct plugin_array *plugins, char *arg,
+				 const char *plugin_dir)
+{
+	char *path = NULL;
+	char *key = NULL;
+	char *value = NULL;
+	struct plugin *pp;
+	struct plugin p;
+	struct plugin_arg a;
+
+	parse_plugin_info(arg, plugin_dir, &path, &key, &value);
+
+	if (path == NULL)
+		return;
+
+	a = (struct plugin_arg){ .key = key, .value = value };
+	pp = get_plugin_with_path(plugins, path);
+
+	if (pp == NULL) {
+		p = (struct plugin){ .path = path, .args = empty_array };
+
+		if (key != NULL)
+			array_add(&p.args, a);
+
+		array_add(plugins, p);
+		return;
+	}
+
+	if (key != NULL)
+		array_add(&pp->args, a);
+
+	free(path);
+}
+
+static void load_plugins(struct plugin_array *plugins,
+			 const struct string_array *plugin_args,
+			 const char *plugin_dir)
+{
+	init_fn_t *init;
+	struct plugin *p;
+	char **arg;
+
+	array_for_each(plugin_args, arg) {
+		register_plugin_info(plugins, *arg, plugin_dir);
+	}
+
+	array_for_each(plugins, p) {
+		p->handle = dlopen(p->path, RTLD_NOW | RTLD_GLOBAL | RTLD_DEEPBIND);
+		if (p->handle == NULL)
+			die("Couldn't load plugin %s: %s\n", p->path, dlerror());
+
+		init = GET_SYMBOL(p, init_fn_t);
+		if (init == NULL)
+			die("Plugin %s needs to export function of type init_fn_t\n",
+			    p->path);
+
+		if ((*init)(DTC_PLUGIN_API_VERSION, p->args.len, p->args.data)) {
+			fprintf(stderr, "DTC: Plugin %s wasn't initialized. Exiting DTC.\n",
+				p->path);
+			exit(1);
+		}
+	}
+}
+
 int main(int argc, char *argv[])
 {
 	struct dt_info *dti;
@@ -170,6 +288,10 @@ int main(int argc, char *argv[])
 	FILE *outf = NULL;
 	int outversion = DEFAULT_FDT_VERSION;
 	long long cmdline_boot_cpuid = -1;
+	struct plugin_array plugins = empty_array;
+	struct plugin *plugin;
+	struct string_array plugin_args = empty_array;
+	const char *plugin_dir = "";
 
 	quiet      = 0;
 	reservenum = 0;
@@ -194,6 +316,12 @@ int main(int argc, char *argv[])
 		case 'd':
 			depname = optarg;
 			break;
+		case 'l':
+			array_add(&plugin_args, optarg);
+			break;
+		case 'L':
+			plugin_dir = optarg;
+			break;
 		case 'R':
 			reservenum = strtol(optarg, NULL, 0);
 			break;
@@ -283,6 +411,8 @@ int main(int argc, char *argv[])
 		fprintf(depfile, "%s:", outname);
 	}
 
+	load_plugins(&plugins, &plugin_args, plugin_dir);
+
 	if (inform == NULL)
 		inform = guess_input_format(arg, "dts");
 	if (outform == NULL) {
@@ -324,6 +454,12 @@ int main(int argc, char *argv[])
 
 	process_checks(force, dti);
 
+	array_for_each(&plugins, plugin) {
+		validate_fn_t *validate = GET_SYMBOL(plugin, validate_fn_t);
+		if (validate)
+			(*validate)(dti);
+	}
+
 	if (auto_label_aliases)
 		generate_label_tree(dti, "aliases", false);
 
@@ -365,5 +501,6 @@ int main(int argc, char *argv[])
 		die("Unknown output format \"%s\"\n", outform);
 	}
 
+	/* Leak the plugins and the live tree */
 	exit(0);
 }
diff --git a/dtc.h b/dtc.h
index f31aed7..4b6280e 100644
--- a/dtc.h
+++ b/dtc.h
@@ -186,4 +186,51 @@ void dt_to_yaml(FILE *f, struct dt_info *dti);
 
 struct dt_info *dt_from_fs(const char *dirname);
 
+/* Plugins */
+
+struct plugin_arg_array {
+	size_t cap;
+	size_t len;
+	struct plugin_arg *data;
+};
+
+struct plugin {
+	const char *path;
+	struct plugin_arg_array args;
+	void *handle;
+};
+
+struct plugin_array {
+	size_t cap;
+	size_t len;
+	struct plugin *data;
+};
+
+struct string_array {
+	size_t cap;
+	size_t len;
+	char **data;
+};
+
+#define empty_array { 0 }
+
+/* Don't add elements to an array while iterating over it */
+#define array_for_each(a, p) \
+	for ((p) = (a)->data; (p) < (a)->data + (a)->len; (p)++)
+
+/* Invalidates all pointers to array members because of the realloc */
+#define array_add(a, p) (						\
+{									\
+	if ((a)->len == (a)->cap) {					\
+		(a)->cap = (a)->cap * 2 + 1;				\
+		(a)->data = xrealloc((a)->data, (a)->cap * sizeof(p));	\
+	}								\
+	(a)->data[(a)->len++] = (p);					\
+})
+
+#define GET_SYMBOL(plugin, type) ((type *)dlsym((plugin)->handle, stringify(TYPE_TO_NAME(type))))
+
+#define DIR_SEPARATOR '/'
+#define SHAREDLIB_EXT ".so"
+
 #endif /* DTC_H */
-- 
2.17.1

Re: [RFC PATCH 4/4] dtc: Add a plugin interface

[Rob Herring]

On Tue, Jul 21, 2020 at 9:59 AM Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org> wrote:
>
> Add support for building and loading plugins.
>
> A plugin interface makes it possible to add checks in any language. It
> also allows these checks to print dts source line information.
>
> Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
> ---
>  Makefile     |  32 +++++++++++-
>  dtc-plugin.h |  27 ++++++++++
>  dtc.c        | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  dtc.h        |  47 +++++++++++++++++
>  4 files changed, 243 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index cb256e8..76bba53 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -25,6 +25,8 @@ WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
>         -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
>  CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
>
> +LDLIBS_dtc += -ldl -export-dynamic
> +
>  BISON = bison
>  LEX = flex
>  SWIG = swig
> @@ -65,14 +67,17 @@ ifeq ($(HOSTOS),darwin)
>  SHAREDLIB_EXT     = dylib
>  SHAREDLIB_CFLAGS  = -fPIC
>  SHAREDLIB_LDFLAGS = -fPIC -dynamiclib -Wl,-install_name -Wl,
> +PLUGIN_ldflags = -fPIC -dynamiclib
>  else ifeq ($(HOSTOS),$(filter $(HOSTOS),msys cygwin))
>  SHAREDLIB_EXT     = so
>  SHAREDLIB_CFLAGS  =
>  SHAREDLIB_LDFLAGS = -shared -Wl,--version-script=$(LIBFDT_version) -Wl,-soname,
> +PLUGIN_ldflags = -shared
>  else
>  SHAREDLIB_EXT     = so
>  SHAREDLIB_CFLAGS  = -fPIC
>  SHAREDLIB_LDFLAGS = -fPIC -shared -Wl,--version-script=$(LIBFDT_version) -Wl,-soname,
> +PLUGIN_ldflags = -fPIC -shared
>  endif
>
>  #
> @@ -186,7 +191,32 @@ ifneq ($(MAKECMDGOALS),libfdt)
>  endif
>  endif
>
> +#
> +# Rules for plugins
> +#
> +PLUGIN_dir = plugins
> +PLUGIN_subdirs = $(patsubst %/,%,$(filter-out $(PLUGIN_dir)/,$(dir $(wildcard $(PLUGIN_dir)/*/))))
> +PLUGIN_names = $(notdir $(PLUGIN_subdirs))
> +PLUGIN_libs = $(join $(PLUGIN_subdirs),$(patsubst %,/%.$(SHAREDLIB_EXT),$(PLUGIN_names)))
> +PLUGIN_CLEANFILES += $(PLUGIN_dir)/*.$(SHAREDLIB_EXT)
> +PLUGIN_CLEANFILES += $(addprefix $(PLUGIN_dir)/*/,*.o *.$(SHAREDLIB_EXT))
> +
> +include $(wildcard $(PLUGIN_dir)/*/Makefile.*)
> +
> +plugins: $(PLUGIN_libs)
> +
> +$(PLUGIN_dir)/%.$(SHAREDLIB_EXT): $(PLUGIN_dir)/%.o
> +       @$(VECHO) LD $@
> +       $(LINK.c) $(PLUGIN_ldflags) -o $@ $< $(PLUGIN_LDLIBS_$(notdir $*))
> +       ln -sf $(patsubst $(PLUGIN_dir)/%,%,$@) $(PLUGIN_dir)/$(notdir $@)
> +
> +$(PLUGIN_dir)/%.o: $(PLUGIN_dir)/%.c
> +       @$(VECHO) CC $@
> +       $(CC) $(CPPFLAGS) $(CFLAGS) $(PLUGIN_CFLAGS_$(notdir $*)) -o $@ -c $<
>
> +plugins_clean:
> +       @$(VECHO) CLEAN "(plugins)"
> +       rm -f $(PLUGIN_CLEANFILES)
>
>  #
>  # Rules for libfdt
> @@ -330,7 +360,7 @@ endif
>  STD_CLEANFILES = *~ *.o *.$(SHAREDLIB_EXT) *.d *.a *.i *.s core a.out vgcore.* \
>         *.tab.[ch] *.lex.c *.output
>
> -clean: libfdt_clean pylibfdt_clean tests_clean
> +clean: libfdt_clean pylibfdt_clean tests_clean plugins_clean
>         @$(VECHO) CLEAN
>         rm -f $(STD_CLEANFILES)
>         rm -f $(VERSION_FILE)
> diff --git a/dtc-plugin.h b/dtc-plugin.h
> index 35c3d19..9ec6b81 100644
> --- a/dtc-plugin.h
> +++ b/dtc-plugin.h
> @@ -10,6 +10,8 @@
>  #include <stdint.h>
>  #include <stdbool.h>
>
> +#include "srcpos.h"
> +

This belongs in patch 3.

>  struct dt_info {
>         const char *version;

Okay, so we don't need this as you are checking the version in a different way.

>         unsigned int dtsflags;
> @@ -170,4 +172,29 @@ static inline size_t type_marker_length(struct marker *m)
>         return 0;
>  }
>
> +struct plugin_arg {
> +       char *key;      /* A non-empty string */
> +       char *value;    /* NULL or a non-empty string */
> +};
> +
> +struct plugin_version {
> +       int major;      /* Incompatible changes */
> +       int minor;      /* Somewhat incompatible changes */

A minor bump should be compatible changes. A new field added to the
end of a struct for example.

> +};
> +
> +#define DTC_PLUGIN_API_VERSION ((struct plugin_version){ .major = 0, .minor = 0 })
> +
> +static inline bool dtc_plugin_default_version_check(struct plugin_version v)

Maybe 'dtc_ver' instead of 'v'.

> +{
> +       struct plugin_version this_v = DTC_PLUGIN_API_VERSION;
> +       return v.major == this_v.major && v.minor == this_v.minor;

I think, by default, minor version differences should be okay.

> +}
> +
> +/* Hook definitions */
> +typedef int (*init_fn_t)(struct plugin_version v, int argc, struct plugin_arg *argv);
> +typedef void (*validate_fn_t)(struct dt_info *dti);
> +
> +#define TYPE_TO_NAME(type) dtc_plugin_v0_##type
> +#define EXPORT_FUNCTION(type, fn) type TYPE_TO_NAME(type) = (fn)
> +
>  #endif /* DTC_PLUGIN_H */
> diff --git a/dtc.c b/dtc.c
> index bdb3f59..5a9b118 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -4,6 +4,7 @@
>   */
>
>  #include <sys/stat.h>
> +#include <dlfcn.h>
>
>  #include "dtc.h"
>  #include "srcpos.h"
> @@ -47,7 +48,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
>
>  /* Usage related data. */
>  static const char usage_synopsis[] = "dtc [options] <input file>";
> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AThv";
> +static const char usage_short_opts[] = "qI:O:o:V:d:l:L:R:S:p:a:fb:i:H:sW:E:@AThv";
>  static struct option const usage_long_opts[] = {
>         {"quiet",            no_argument, NULL, 'q'},
>         {"in-format",         a_argument, NULL, 'I'},
> @@ -55,6 +56,8 @@ static struct option const usage_long_opts[] = {
>         {"out-format",        a_argument, NULL, 'O'},
>         {"out-version",       a_argument, NULL, 'V'},
>         {"out-dependency",    a_argument, NULL, 'd'},
> +       {"plugin",            a_argument, NULL, 'l'},
> +       {"plugin-dir",        a_argument, NULL, 'L'},
>         {"reserve",           a_argument, NULL, 'R'},
>         {"space",             a_argument, NULL, 'S'},
>         {"pad",               a_argument, NULL, 'p'},
> @@ -89,6 +92,13 @@ static const char * const usage_opts_help[] = {
>          "\t\tasm - assembler source",
>         "\n\tBlob version to produce, defaults to "stringify(DEFAULT_FDT_VERSION)" (for dtb and asm output)",
>         "\n\tOutput dependency file",
> +       "\n\tLoad a plugin and, optionally, pass it an argument.\n"
> +       "\tUsage: -l <plugin name>[,key][,value]\n"
> +        "\t\tplugin name - the name of the shared object without the extension (can contain a path)\n"
> +        "\t\tkey         - the name of the argument\n"
> +        "\t\tvalue       - can be omitted\n"
> +       "\tExample: dtc -lsome-plugin,o,out.dts -lsome-plugin,help -l path/to/another-plugin [...]",

Should be '-L':

-L path/to/another-plugin

> +       "\n\tThe directory in which to search for plugins",
>         "\n\tMake space for <number> reserve map entries (for dtb and asm output)",
>         "\n\tMake the blob at least <bytes> long (extra space)",
>         "\n\tAdd padding to the blob of <bytes> long (extra space)",
> @@ -157,6 +167,114 @@ static const char *guess_input_format(const char *fname, const char *fallback)
>         return guess_type_by_name(fname, fallback);
>  }
>
> +static struct plugin *get_plugin_with_path(struct plugin_array *plugins,
> +                                          char *path)
> +{
> +       struct plugin *p;
> +       array_for_each(plugins, p)
> +               if (strcmp(p->path, path) == 0)
> +                       return p;
> +
> +       return NULL;
> +}
> +
> +static void parse_plugin_info(char *arg, const char *plugin_dir, char **path,
> +                             char **key, char **value)
> +{
> +       char *name;
> +       size_t dirlen;
> +       char *tmp;
> +       size_t tmplen;
> +
> +       if (arg == NULL || *arg == '\0' || *arg == ',')
> +               return;
> +
> +       name = strtok(arg, ",");
> +
> +       dirlen = strlen(plugin_dir);
> +       tmplen = dirlen + strlen(name) + strlen(SHAREDLIB_EXT) + 2;
> +       tmp = xmalloc(tmplen);
> +
> +       if (plugin_dir[0] == '\0' || plugin_dir[dirlen - 1] == DIR_SEPARATOR)
> +               snprintf(tmp, tmplen, "%s%s%s", plugin_dir,
> +                        name, SHAREDLIB_EXT);
> +       else
> +               snprintf(tmp, tmplen, "%s%c%s%s", plugin_dir,
> +                        DIR_SEPARATOR, name, SHAREDLIB_EXT);
> +
> +       *path = realpath(tmp, NULL); /* malloc path */
> +       if (*path == NULL)
> +               die("Couldn't resolve plugin path %s: %s\n", tmp, strerror(errno));
> +
> +       *key = strtok(NULL, ",");
> +       *value = strtok(NULL, "");
> +       free(tmp);
> +}
> +
> +static void register_plugin_info(struct plugin_array *plugins, char *arg,
> +                                const char *plugin_dir)
> +{
> +       char *path = NULL;
> +       char *key = NULL;
> +       char *value = NULL;
> +       struct plugin *pp;
> +       struct plugin p;
> +       struct plugin_arg a;
> +
> +       parse_plugin_info(arg, plugin_dir, &path, &key, &value);
> +
> +       if (path == NULL)
> +               return;
> +
> +       a = (struct plugin_arg){ .key = key, .value = value };
> +       pp = get_plugin_with_path(plugins, path);
> +
> +       if (pp == NULL) {
> +               p = (struct plugin){ .path = path, .args = empty_array };
> +
> +               if (key != NULL)
> +                       array_add(&p.args, a);
> +
> +               array_add(plugins, p);
> +               return;
> +       }
> +
> +       if (key != NULL)
> +               array_add(&pp->args, a);
> +
> +       free(path);
> +}
> +
> +static void load_plugins(struct plugin_array *plugins,
> +                        const struct string_array *plugin_args,
> +                        const char *plugin_dir)
> +{
> +       init_fn_t *init;
> +       struct plugin *p;
> +       char **arg;
> +
> +       array_for_each(plugin_args, arg) {
> +               register_plugin_info(plugins, *arg, plugin_dir);
> +       }
> +
> +       array_for_each(plugins, p) {
> +               p->handle = dlopen(p->path, RTLD_NOW | RTLD_GLOBAL | RTLD_DEEPBIND);
> +               if (p->handle == NULL)
> +                       die("Couldn't load plugin %s: %s\n", p->path, dlerror());
> +
> +               init = GET_SYMBOL(p, init_fn_t);
> +               if (init == NULL)
> +                       die("Plugin %s needs to export function of type init_fn_t\n",
> +                           p->path);
> +
> +               if ((*init)(DTC_PLUGIN_API_VERSION, p->args.len, p->args.data)) {
> +                       fprintf(stderr, "DTC: Plugin %s wasn't initialized. Exiting DTC.\n",
> +                               p->path);
> +                       exit(1);
> +               }
> +       }
> +}
> +
>  int main(int argc, char *argv[])
>  {
>         struct dt_info *dti;
> @@ -170,6 +288,10 @@ int main(int argc, char *argv[])
>         FILE *outf = NULL;
>         int outversion = DEFAULT_FDT_VERSION;
>         long long cmdline_boot_cpuid = -1;
> +       struct plugin_array plugins = empty_array;
> +       struct plugin *plugin;
> +       struct string_array plugin_args = empty_array;
> +       const char *plugin_dir = "";
>
>         quiet      = 0;
>         reservenum = 0;
> @@ -194,6 +316,12 @@ int main(int argc, char *argv[])
>                 case 'd':
>                         depname = optarg;
>                         break;
> +               case 'l':
> +                       array_add(&plugin_args, optarg);
> +                       break;
> +               case 'L':
> +                       plugin_dir = optarg;
> +                       break;
>                 case 'R':
>                         reservenum = strtol(optarg, NULL, 0);
>                         break;
> @@ -283,6 +411,8 @@ int main(int argc, char *argv[])
>                 fprintf(depfile, "%s:", outname);
>         }
>
> +       load_plugins(&plugins, &plugin_args, plugin_dir);
> +
>         if (inform == NULL)
>                 inform = guess_input_format(arg, "dts");
>         if (outform == NULL) {
> @@ -324,6 +454,12 @@ int main(int argc, char *argv[])
>
>         process_checks(force, dti);
>
> +       array_for_each(&plugins, plugin) {
> +               validate_fn_t *validate = GET_SYMBOL(plugin, validate_fn_t);
> +               if (validate)
> +                       (*validate)(dti);
> +       }
> +
>         if (auto_label_aliases)
>                 generate_label_tree(dti, "aliases", false);
>
> @@ -365,5 +501,6 @@ int main(int argc, char *argv[])
>                 die("Unknown output format \"%s\"\n", outform);
>         }
>
> +       /* Leak the plugins and the live tree */
>         exit(0);
>  }
> diff --git a/dtc.h b/dtc.h
> index f31aed7..4b6280e 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -186,4 +186,51 @@ void dt_to_yaml(FILE *f, struct dt_info *dti);
>
>  struct dt_info *dt_from_fs(const char *dirname);
>
> +/* Plugins */
> +
> +struct plugin_arg_array {
> +       size_t cap;
> +       size_t len;
> +       struct plugin_arg *data;
> +};
> +
> +struct plugin {
> +       const char *path;
> +       struct plugin_arg_array args;
> +       void *handle;
> +};
> +
> +struct plugin_array {
> +       size_t cap;
> +       size_t len;
> +       struct plugin *data;
> +};
> +
> +struct string_array {
> +       size_t cap;
> +       size_t len;
> +       char **data;
> +};
> +
> +#define empty_array { 0 }
> +
> +/* Don't add elements to an array while iterating over it */
> +#define array_for_each(a, p) \
> +       for ((p) = (a)->data; (p) < (a)->data + (a)->len; (p)++)
> +
> +/* Invalidates all pointers to array members because of the realloc */
> +#define array_add(a, p) (                                              \
> +{                                                                      \
> +       if ((a)->len == (a)->cap) {                                     \
> +               (a)->cap = (a)->cap * 2 + 1;                            \
> +               (a)->data = xrealloc((a)->data, (a)->cap * sizeof(p));  \
> +       }                                                               \
> +       (a)->data[(a)->len++] = (p);                                    \
> +})
> +
> +#define GET_SYMBOL(plugin, type) ((type *)dlsym((plugin)->handle, stringify(TYPE_TO_NAME(type))))
> +
> +#define DIR_SEPARATOR '/'
> +#define SHAREDLIB_EXT ".so"
> +
>  #endif /* DTC_H */
> --
> 2.17.1
>

Re: [RFC PATCH 4/4] dtc: Add a plugin interface

[Andrei Ziureaev]

On 7/23/20 3:46 PM, Rob Herring wrote:
> On Tue, Jul 21, 2020 at 9:59 AM Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org> wrote:
>> Add support for building and loading plugins.
>>
>> A plugin interface makes it possible to add checks in any language. It
>> also allows these checks to print dts source line information.
>>
>> Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
>> ---
>>   Makefile     |  32 +++++++++++-
>>   dtc-plugin.h |  27 ++++++++++
>>   dtc.c        | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   dtc.h        |  47 +++++++++++++++++
>>   4 files changed, 243 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index cb256e8..76bba53 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -25,6 +25,8 @@ WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
>>          -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
>>   CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
>>
>> +LDLIBS_dtc += -ldl -export-dynamic
>> +
>>   BISON = bison
>>   LEX = flex
>>   SWIG = swig
>> @@ -65,14 +67,17 @@ ifeq ($(HOSTOS),darwin)
>>   SHAREDLIB_EXT     = dylib
>>   SHAREDLIB_CFLAGS  = -fPIC
>>   SHAREDLIB_LDFLAGS = -fPIC -dynamiclib -Wl,-install_name -Wl,
>> +PLUGIN_ldflags = -fPIC -dynamiclib
>>   else ifeq ($(HOSTOS),$(filter $(HOSTOS),msys cygwin))
>>   SHAREDLIB_EXT     = so
>>   SHAREDLIB_CFLAGS  =
>>   SHAREDLIB_LDFLAGS = -shared -Wl,--version-script=$(LIBFDT_version) -Wl,-soname,
>> +PLUGIN_ldflags = -shared
>>   else
>>   SHAREDLIB_EXT     = so
>>   SHAREDLIB_CFLAGS  = -fPIC
>>   SHAREDLIB_LDFLAGS = -fPIC -shared -Wl,--version-script=$(LIBFDT_version) -Wl,-soname,
>> +PLUGIN_ldflags = -fPIC -shared
>>   endif
>>
>>   #
>> @@ -186,7 +191,32 @@ ifneq ($(MAKECMDGOALS),libfdt)
>>   endif
>>   endif
>>
>> +#
>> +# Rules for plugins
>> +#
>> +PLUGIN_dir = plugins
>> +PLUGIN_subdirs = $(patsubst %/,%,$(filter-out $(PLUGIN_dir)/,$(dir $(wildcard $(PLUGIN_dir)/*/))))
>> +PLUGIN_names = $(notdir $(PLUGIN_subdirs))
>> +PLUGIN_libs = $(join $(PLUGIN_subdirs),$(patsubst %,/%.$(SHAREDLIB_EXT),$(PLUGIN_names)))
>> +PLUGIN_CLEANFILES += $(PLUGIN_dir)/*.$(SHAREDLIB_EXT)
>> +PLUGIN_CLEANFILES += $(addprefix $(PLUGIN_dir)/*/,*.o *.$(SHAREDLIB_EXT))
>> +
>> +include $(wildcard $(PLUGIN_dir)/*/Makefile.*)
>> +
>> +plugins: $(PLUGIN_libs)
>> +
>> +$(PLUGIN_dir)/%.$(SHAREDLIB_EXT): $(PLUGIN_dir)/%.o
>> +       @$(VECHO) LD $@
>> +       $(LINK.c) $(PLUGIN_ldflags) -o $@ $< $(PLUGIN_LDLIBS_$(notdir $*))
>> +       ln -sf $(patsubst $(PLUGIN_dir)/%,%,$@) $(PLUGIN_dir)/$(notdir $@)
>> +
>> +$(PLUGIN_dir)/%.o: $(PLUGIN_dir)/%.c
>> +       @$(VECHO) CC $@
>> +       $(CC) $(CPPFLAGS) $(CFLAGS) $(PLUGIN_CFLAGS_$(notdir $*)) -o $@ -c $<
>>
>> +plugins_clean:
>> +       @$(VECHO) CLEAN "(plugins)"
>> +       rm -f $(PLUGIN_CLEANFILES)
>>
>>   #
>>   # Rules for libfdt
>> @@ -330,7 +360,7 @@ endif
>>   STD_CLEANFILES = *~ *.o *.$(SHAREDLIB_EXT) *.d *.a *.i *.s core a.out vgcore.* \
>>          *.tab.[ch] *.lex.c *.output
>>
>> -clean: libfdt_clean pylibfdt_clean tests_clean
>> +clean: libfdt_clean pylibfdt_clean tests_clean plugins_clean
>>          @$(VECHO) CLEAN
>>          rm -f $(STD_CLEANFILES)
>>          rm -f $(VERSION_FILE)
>> diff --git a/dtc-plugin.h b/dtc-plugin.h
>> index 35c3d19..9ec6b81 100644
>> --- a/dtc-plugin.h
>> +++ b/dtc-plugin.h
>> @@ -10,6 +10,8 @@
>>   #include <stdint.h>
>>   #include <stdbool.h>
>>
>> +#include "srcpos.h"
>> +
> This belongs in patch 3.
>
>>   struct dt_info {
>>          const char *version;
> Okay, so we don't need this as you are checking the version in a different way.


Oops, I forgot to remove this.

>
>>          unsigned int dtsflags;
>> @@ -170,4 +172,29 @@ static inline size_t type_marker_length(struct marker *m)
>>          return 0;
>>   }
>>
>> +struct plugin_arg {
>> +       char *key;      /* A non-empty string */
>> +       char *value;    /* NULL or a non-empty string */
>> +};
>> +
>> +struct plugin_version {
>> +       int major;      /* Incompatible changes */
>> +       int minor;      /* Somewhat incompatible changes */
> A minor bump should be compatible changes. A new field added to the
> end of a struct for example.
>
>> +};
>> +
>> +#define DTC_PLUGIN_API_VERSION ((struct plugin_version){ .major = 0, .minor = 0 })
>> +
>> +static inline bool dtc_plugin_default_version_check(struct plugin_version v)
> Maybe 'dtc_ver' instead of 'v'.
>
>> +{
>> +       struct plugin_version this_v = DTC_PLUGIN_API_VERSION;
>> +       return v.major == this_v.major && v.minor == this_v.minor;
> I think, by default, minor version differences should be okay.

I originally wanted this whole interface to be similar to gcc's (it
turned out nothing like gcc's), and gcc's default version check is the
strictest check possible. I guess the idea is that plugin authors can
write their own less strict checks if they want. But if minor versions
are compatible, then maybe there's no point in checking them, even by
default.

>
>> +}
>> +
>> +/* Hook definitions */
>> +typedef int (*init_fn_t)(struct plugin_version v, int argc, struct plugin_arg *argv);
>> +typedef void (*validate_fn_t)(struct dt_info *dti);
>> +
>> +#define TYPE_TO_NAME(type) dtc_plugin_v0_##type
>> +#define EXPORT_FUNCTION(type, fn) type TYPE_TO_NAME(type) = (fn)
>> +
>>   #endif /* DTC_PLUGIN_H */
>> diff --git a/dtc.c b/dtc.c
>> index bdb3f59..5a9b118 100644
>> --- a/dtc.c
>> +++ b/dtc.c
>> @@ -4,6 +4,7 @@
>>    */
>>
>>   #include <sys/stat.h>
>> +#include <dlfcn.h>
>>
>>   #include "dtc.h"
>>   #include "srcpos.h"
>> @@ -47,7 +48,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
>>
>>   /* Usage related data. */
>>   static const char usage_synopsis[] = "dtc [options] <input file>";
>> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AThv";
>> +static const char usage_short_opts[] = "qI:O:o:V:d:l:L:R:S:p:a:fb:i:H:sW:E:@AThv";
>>   static struct option const usage_long_opts[] = {
>>          {"quiet",            no_argument, NULL, 'q'},
>>          {"in-format",         a_argument, NULL, 'I'},
>> @@ -55,6 +56,8 @@ static struct option const usage_long_opts[] = {
>>          {"out-format",        a_argument, NULL, 'O'},
>>          {"out-version",       a_argument, NULL, 'V'},
>>          {"out-dependency",    a_argument, NULL, 'd'},
>> +       {"plugin",            a_argument, NULL, 'l'},
>> +       {"plugin-dir",        a_argument, NULL, 'L'},
>>          {"reserve",           a_argument, NULL, 'R'},
>>          {"space",             a_argument, NULL, 'S'},
>>          {"pad",               a_argument, NULL, 'p'},
>> @@ -89,6 +92,13 @@ static const char * const usage_opts_help[] = {
>>           "\t\tasm - assembler source",
>>          "\n\tBlob version to produce, defaults to "stringify(DEFAULT_FDT_VERSION)" (for dtb and asm output)",
>>          "\n\tOutput dependency file",
>> +       "\n\tLoad a plugin and, optionally, pass it an argument.\n"
>> +       "\tUsage: -l <plugin name>[,key][,value]\n"
>> +        "\t\tplugin name - the name of the shared object without the extension (can contain a path)\n"
>> +        "\t\tkey         - the name of the argument\n"
>> +        "\t\tvalue       - can be omitted\n"
>> +       "\tExample: dtc -lsome-plugin,o,out.dts -lsome-plugin,help -l path/to/another-plugin [...]",
> Should be '-L':
>
> -L path/to/another-plugin


"-L" specifies the directory and "-l" - a plugin name with an optional
path and an argument. I guess we can simplify this, get rid of "-l", and
just have "-L" take a full path and an argument. Since this is going to
be called mostly from makefiles, the extra typing shouldn't be a
problem. We could also pass all the arguments at once with something like:

-L path/plugin-name,"key=value key2=value2"

I do, however, want the plugins to receive key-value pairs and not have
to parse the arguments themselves.

>
>> +       "\n\tThe directory in which to search for plugins",
>>          "\n\tMake space for <number> reserve map entries (for dtb and asm output)",
>>          "\n\tMake the blob at least <bytes> long (extra space)",
>>          "\n\tAdd padding to the blob of <bytes> long (extra space)",
>> @@ -157,6 +167,114 @@ static const char *guess_input_format(const char *fname, const char *fallback)
>>          return guess_type_by_name(fname, fallback);
>>   }
>>
>> +static struct plugin *get_plugin_with_path(struct plugin_array *plugins,
>> +                                          char *path)
>> +{
>> +       struct plugin *p;
>> +       array_for_each(plugins, p)
>> +               if (strcmp(p->path, path) == 0)
>> +                       return p;
>> +
>> +       return NULL;
>> +}
>> +
>> +static void parse_plugin_info(char *arg, const char *plugin_dir, char **path,
>> +                             char **key, char **value)
>> +{
>> +       char *name;
>> +       size_t dirlen;
>> +       char *tmp;
>> +       size_t tmplen;
>> +
>> +       if (arg == NULL || *arg == '\0' || *arg == ',')
>> +               return;
>> +
>> +       name = strtok(arg, ",");
>> +
>> +       dirlen = strlen(plugin_dir);
>> +       tmplen = dirlen + strlen(name) + strlen(SHAREDLIB_EXT) + 2;
>> +       tmp = xmalloc(tmplen);
>> +
>> +       if (plugin_dir[0] == '\0' || plugin_dir[dirlen - 1] == DIR_SEPARATOR)
>> +               snprintf(tmp, tmplen, "%s%s%s", plugin_dir,
>> +                        name, SHAREDLIB_EXT);
>> +       else
>> +               snprintf(tmp, tmplen, "%s%c%s%s", plugin_dir,
>> +                        DIR_SEPARATOR, name, SHAREDLIB_EXT);
>> +
>> +       *path = realpath(tmp, NULL); /* malloc path */
>> +       if (*path == NULL)
>> +               die("Couldn't resolve plugin path %s: %s\n", tmp, strerror(errno));
>> +
>> +       *key = strtok(NULL, ",");
>> +       *value = strtok(NULL, "");
>> +       free(tmp);
>> +}
>> +
>> +static void register_plugin_info(struct plugin_array *plugins, char *arg,
>> +                                const char *plugin_dir)
>> +{
>> +       char *path = NULL;
>> +       char *key = NULL;
>> +       char *value = NULL;
>> +       struct plugin *pp;
>> +       struct plugin p;
>> +       struct plugin_arg a;
>> +
>> +       parse_plugin_info(arg, plugin_dir, &path, &key, &value);
>> +
>> +       if (path == NULL)
>> +               return;
>> +
>> +       a = (struct plugin_arg){ .key = key, .value = value };
>> +       pp = get_plugin_with_path(plugins, path);
>> +
>> +       if (pp == NULL) {
>> +               p = (struct plugin){ .path = path, .args = empty_array };
>> +
>> +               if (key != NULL)
>> +                       array_add(&p.args, a);
>> +
>> +               array_add(plugins, p);
>> +               return;
>> +       }
>> +
>> +       if (key != NULL)
>> +               array_add(&pp->args, a);
>> +
>> +       free(path);
>> +}
>> +
>> +static void load_plugins(struct plugin_array *plugins,
>> +                        const struct string_array *plugin_args,
>> +                        const char *plugin_dir)
>> +{
>> +       init_fn_t *init;
>> +       struct plugin *p;
>> +       char **arg;
>> +
>> +       array_for_each(plugin_args, arg) {
>> +               register_plugin_info(plugins, *arg, plugin_dir);
>> +       }
>> +
>> +       array_for_each(plugins, p) {
>> +               p->handle = dlopen(p->path, RTLD_NOW | RTLD_GLOBAL | RTLD_DEEPBIND);
>> +               if (p->handle == NULL)
>> +                       die("Couldn't load plugin %s: %s\n", p->path, dlerror());
>> +
>> +               init = GET_SYMBOL(p, init_fn_t);
>> +               if (init == NULL)
>> +                       die("Plugin %s needs to export function of type init_fn_t\n",
>> +                           p->path);
>> +
>> +               if ((*init)(DTC_PLUGIN_API_VERSION, p->args.len, p->args.data)) {
>> +                       fprintf(stderr, "DTC: Plugin %s wasn't initialized. Exiting DTC.\n",
>> +                               p->path);
>> +                       exit(1);
>> +               }
>> +       }
>> +}
>> +
>>   int main(int argc, char *argv[])
>>   {
>>          struct dt_info *dti;
>> @@ -170,6 +288,10 @@ int main(int argc, char *argv[])
>>          FILE *outf = NULL;
>>          int outversion = DEFAULT_FDT_VERSION;
>>          long long cmdline_boot_cpuid = -1;
>> +       struct plugin_array plugins = empty_array;
>> +       struct plugin *plugin;
>> +       struct string_array plugin_args = empty_array;
>> +       const char *plugin_dir = "";
>>
>>          quiet      = 0;
>>          reservenum = 0;
>> @@ -194,6 +316,12 @@ int main(int argc, char *argv[])
>>                  case 'd':
>>                          depname = optarg;
>>                          break;
>> +               case 'l':
>> +                       array_add(&plugin_args, optarg);
>> +                       break;
>> +               case 'L':
>> +                       plugin_dir = optarg;
>> +                       break;
>>                  case 'R':
>>                          reservenum = strtol(optarg, NULL, 0);
>>                          break;
>> @@ -283,6 +411,8 @@ int main(int argc, char *argv[])
>>                  fprintf(depfile, "%s:", outname);
>>          }
>>
>> +       load_plugins(&plugins, &plugin_args, plugin_dir);
>> +
>>          if (inform == NULL)
>>                  inform = guess_input_format(arg, "dts");
>>          if (outform == NULL) {
>> @@ -324,6 +454,12 @@ int main(int argc, char *argv[])
>>
>>          process_checks(force, dti);
>>
>> +       array_for_each(&plugins, plugin) {
>> +               validate_fn_t *validate = GET_SYMBOL(plugin, validate_fn_t);
>> +               if (validate)
>> +                       (*validate)(dti);
>> +       }
>> +
>>          if (auto_label_aliases)
>>                  generate_label_tree(dti, "aliases", false);
>>
>> @@ -365,5 +501,6 @@ int main(int argc, char *argv[])
>>                  die("Unknown output format \"%s\"\n", outform);
>>          }
>>
>> +       /* Leak the plugins and the live tree */
>>          exit(0);
>>   }
>> diff --git a/dtc.h b/dtc.h
>> index f31aed7..4b6280e 100644
>> --- a/dtc.h
>> +++ b/dtc.h
>> @@ -186,4 +186,51 @@ void dt_to_yaml(FILE *f, struct dt_info *dti);
>>
>>   struct dt_info *dt_from_fs(const char *dirname);
>>
>> +/* Plugins */
>> +
>> +struct plugin_arg_array {
>> +       size_t cap;
>> +       size_t len;
>> +       struct plugin_arg *data;
>> +};
>> +
>> +struct plugin {
>> +       const char *path;
>> +       struct plugin_arg_array args;
>> +       void *handle;
>> +};
>> +
>> +struct plugin_array {
>> +       size_t cap;
>> +       size_t len;
>> +       struct plugin *data;
>> +};
>> +
>> +struct string_array {
>> +       size_t cap;
>> +       size_t len;
>> +       char **data;
>> +};
>> +
>> +#define empty_array { 0 }
>> +
>> +/* Don't add elements to an array while iterating over it */
>> +#define array_for_each(a, p) \
>> +       for ((p) = (a)->data; (p) < (a)->data + (a)->len; (p)++)
>> +
>> +/* Invalidates all pointers to array members because of the realloc */
>> +#define array_add(a, p) (                                              \
>> +{                                                                      \
>> +       if ((a)->len == (a)->cap) {                                     \
>> +               (a)->cap = (a)->cap * 2 + 1;                            \
>> +               (a)->data = xrealloc((a)->data, (a)->cap * sizeof(p));  \
>> +       }                                                               \
>> +       (a)->data[(a)->len++] = (p);                                    \
>> +})
>> +
>> +#define GET_SYMBOL(plugin, type) ((type *)dlsym((plugin)->handle, stringify(TYPE_TO_NAME(type))))
>> +
>> +#define DIR_SEPARATOR '/'
>> +#define SHAREDLIB_EXT ".so"
>> +
>>   #endif /* DTC_H */
>> --
>> 2.17.1
>>

Re: [RFC PATCH 4/4] dtc: Add a plugin interface

[Rob Herring]

On Thu, Jul 23, 2020 at 12:23 PM Andrei Ziureaev
<andrei.ziureaev-5wv7dgnIgG8@public.gmane.org> wrote:
>
>
> On 7/23/20 3:46 PM, Rob Herring wrote:
> > On Tue, Jul 21, 2020 at 9:59 AM Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org> wrote:
> >> Add support for building and loading plugins.
> >>
> >> A plugin interface makes it possible to add checks in any language. It
> >> also allows these checks to print dts source line information.
> >>
> >> Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
> >> ---
> >>   Makefile     |  32 +++++++++++-
> >>   dtc-plugin.h |  27 ++++++++++
> >>   dtc.c        | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>   dtc.h        |  47 +++++++++++++++++
> >>   4 files changed, 243 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index cb256e8..76bba53 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -25,6 +25,8 @@ WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
> >>          -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
> >>   CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
> >>
> >> +LDLIBS_dtc += -ldl -export-dynamic
> >> +
> >>   BISON = bison
> >>   LEX = flex
> >>   SWIG = swig
> >> @@ -65,14 +67,17 @@ ifeq ($(HOSTOS),darwin)
> >>   SHAREDLIB_EXT     = dylib
> >>   SHAREDLIB_CFLAGS  = -fPIC
> >>   SHAREDLIB_LDFLAGS = -fPIC -dynamiclib -Wl,-install_name -Wl,
> >> +PLUGIN_ldflags = -fPIC -dynamiclib
> >>   else ifeq ($(HOSTOS),$(filter $(HOSTOS),msys cygwin))
> >>   SHAREDLIB_EXT     = so
> >>   SHAREDLIB_CFLAGS  =
> >>   SHAREDLIB_LDFLAGS = -shared -Wl,--version-script=$(LIBFDT_version) -Wl,-soname,
> >> +PLUGIN_ldflags = -shared
> >>   else
> >>   SHAREDLIB_EXT     = so
> >>   SHAREDLIB_CFLAGS  = -fPIC
> >>   SHAREDLIB_LDFLAGS = -fPIC -shared -Wl,--version-script=$(LIBFDT_version) -Wl,-soname,
> >> +PLUGIN_ldflags = -fPIC -shared
> >>   endif
> >>
> >>   #
> >> @@ -186,7 +191,32 @@ ifneq ($(MAKECMDGOALS),libfdt)
> >>   endif
> >>   endif
> >>
> >> +#
> >> +# Rules for plugins
> >> +#
> >> +PLUGIN_dir = plugins
> >> +PLUGIN_subdirs = $(patsubst %/,%,$(filter-out $(PLUGIN_dir)/,$(dir $(wildcard $(PLUGIN_dir)/*/))))
> >> +PLUGIN_names = $(notdir $(PLUGIN_subdirs))
> >> +PLUGIN_libs = $(join $(PLUGIN_subdirs),$(patsubst %,/%.$(SHAREDLIB_EXT),$(PLUGIN_names)))
> >> +PLUGIN_CLEANFILES += $(PLUGIN_dir)/*.$(SHAREDLIB_EXT)
> >> +PLUGIN_CLEANFILES += $(addprefix $(PLUGIN_dir)/*/,*.o *.$(SHAREDLIB_EXT))
> >> +
> >> +include $(wildcard $(PLUGIN_dir)/*/Makefile.*)
> >> +
> >> +plugins: $(PLUGIN_libs)
> >> +
> >> +$(PLUGIN_dir)/%.$(SHAREDLIB_EXT): $(PLUGIN_dir)/%.o
> >> +       @$(VECHO) LD $@
> >> +       $(LINK.c) $(PLUGIN_ldflags) -o $@ $< $(PLUGIN_LDLIBS_$(notdir $*))
> >> +       ln -sf $(patsubst $(PLUGIN_dir)/%,%,$@) $(PLUGIN_dir)/$(notdir $@)
> >> +
> >> +$(PLUGIN_dir)/%.o: $(PLUGIN_dir)/%.c
> >> +       @$(VECHO) CC $@
> >> +       $(CC) $(CPPFLAGS) $(CFLAGS) $(PLUGIN_CFLAGS_$(notdir $*)) -o $@ -c $<
> >>
> >> +plugins_clean:
> >> +       @$(VECHO) CLEAN "(plugins)"
> >> +       rm -f $(PLUGIN_CLEANFILES)
> >>
> >>   #
> >>   # Rules for libfdt
> >> @@ -330,7 +360,7 @@ endif
> >>   STD_CLEANFILES = *~ *.o *.$(SHAREDLIB_EXT) *.d *.a *.i *.s core a.out vgcore.* \
> >>          *.tab.[ch] *.lex.c *.output
> >>
> >> -clean: libfdt_clean pylibfdt_clean tests_clean
> >> +clean: libfdt_clean pylibfdt_clean tests_clean plugins_clean
> >>          @$(VECHO) CLEAN
> >>          rm -f $(STD_CLEANFILES)
> >>          rm -f $(VERSION_FILE)
> >> diff --git a/dtc-plugin.h b/dtc-plugin.h
> >> index 35c3d19..9ec6b81 100644
> >> --- a/dtc-plugin.h
> >> +++ b/dtc-plugin.h
> >> @@ -10,6 +10,8 @@
> >>   #include <stdint.h>
> >>   #include <stdbool.h>
> >>
> >> +#include "srcpos.h"
> >> +
> > This belongs in patch 3.
> >
> >>   struct dt_info {
> >>          const char *version;
> > Okay, so we don't need this as you are checking the version in a different way.
>
>
> Oops, I forgot to remove this.
>
> >
> >>          unsigned int dtsflags;
> >> @@ -170,4 +172,29 @@ static inline size_t type_marker_length(struct marker *m)
> >>          return 0;
> >>   }
> >>
> >> +struct plugin_arg {
> >> +       char *key;      /* A non-empty string */
> >> +       char *value;    /* NULL or a non-empty string */
> >> +};
> >> +
> >> +struct plugin_version {
> >> +       int major;      /* Incompatible changes */
> >> +       int minor;      /* Somewhat incompatible changes */
> > A minor bump should be compatible changes. A new field added to the
> > end of a struct for example.
> >
> >> +};
> >> +
> >> +#define DTC_PLUGIN_API_VERSION ((struct plugin_version){ .major = 0, .minor = 0 })
> >> +
> >> +static inline bool dtc_plugin_default_version_check(struct plugin_version v)
> > Maybe 'dtc_ver' instead of 'v'.
> >
> >> +{
> >> +       struct plugin_version this_v = DTC_PLUGIN_API_VERSION;
> >> +       return v.major == this_v.major && v.minor == this_v.minor;
> > I think, by default, minor version differences should be okay.
>
> I originally wanted this whole interface to be similar to gcc's (it
> turned out nothing like gcc's), and gcc's default version check is the
> strictest check possible. I guess the idea is that plugin authors can
> write their own less strict checks if they want. But if minor versions
> are compatible, then maybe there's no point in checking them, even by
> default.

Okay. Let's see if there's any other opinions.

> >> +}
> >> +
> >> +/* Hook definitions */
> >> +typedef int (*init_fn_t)(struct plugin_version v, int argc, struct plugin_arg *argv);
> >> +typedef void (*validate_fn_t)(struct dt_info *dti);
> >> +
> >> +#define TYPE_TO_NAME(type) dtc_plugin_v0_##type
> >> +#define EXPORT_FUNCTION(type, fn) type TYPE_TO_NAME(type) = (fn)
> >> +
> >>   #endif /* DTC_PLUGIN_H */
> >> diff --git a/dtc.c b/dtc.c
> >> index bdb3f59..5a9b118 100644
> >> --- a/dtc.c
> >> +++ b/dtc.c
> >> @@ -4,6 +4,7 @@
> >>    */
> >>
> >>   #include <sys/stat.h>
> >> +#include <dlfcn.h>
> >>
> >>   #include "dtc.h"
> >>   #include "srcpos.h"
> >> @@ -47,7 +48,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
> >>
> >>   /* Usage related data. */
> >>   static const char usage_synopsis[] = "dtc [options] <input file>";
> >> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AThv";
> >> +static const char usage_short_opts[] = "qI:O:o:V:d:l:L:R:S:p:a:fb:i:H:sW:E:@AThv";
> >>   static struct option const usage_long_opts[] = {
> >>          {"quiet",            no_argument, NULL, 'q'},
> >>          {"in-format",         a_argument, NULL, 'I'},
> >> @@ -55,6 +56,8 @@ static struct option const usage_long_opts[] = {
> >>          {"out-format",        a_argument, NULL, 'O'},
> >>          {"out-version",       a_argument, NULL, 'V'},
> >>          {"out-dependency",    a_argument, NULL, 'd'},
> >> +       {"plugin",            a_argument, NULL, 'l'},
> >> +       {"plugin-dir",        a_argument, NULL, 'L'},
> >>          {"reserve",           a_argument, NULL, 'R'},
> >>          {"space",             a_argument, NULL, 'S'},
> >>          {"pad",               a_argument, NULL, 'p'},
> >> @@ -89,6 +92,13 @@ static const char * const usage_opts_help[] = {
> >>           "\t\tasm - assembler source",
> >>          "\n\tBlob version to produce, defaults to "stringify(DEFAULT_FDT_VERSION)" (for dtb and asm output)",
> >>          "\n\tOutput dependency file",
> >> +       "\n\tLoad a plugin and, optionally, pass it an argument.\n"
> >> +       "\tUsage: -l <plugin name>[,key][,value]\n"
> >> +        "\t\tplugin name - the name of the shared object without the extension (can contain a path)\n"
> >> +        "\t\tkey         - the name of the argument\n"
> >> +        "\t\tvalue       - can be omitted\n"
> >> +       "\tExample: dtc -lsome-plugin,o,out.dts -lsome-plugin,help -l path/to/another-plugin [...]",
> > Should be '-L':
> >
> > -L path/to/another-plugin
>
>
> "-L" specifies the directory and "-l" - a plugin name with an optional
> path and an argument. I guess we can simplify this, get rid of "-l", and
> just have "-L" take a full path and an argument. Since this is going to
> be called mostly from makefiles, the extra typing shouldn't be a
> problem. We could also pass all the arguments at once with something like:
>
> -L path/plugin-name,"key=value key2=value2"

Ah, I was confused thinking that 'path/to/another-plugin' was a path.
I'd just drop 'path/to/' here.

I think a separate search path is better. We could still do all args
together. I don't really have a preference on that. Probably would
want a comma separator rather than a space though.

Rob