[PATCH] Add support for binary includes.

A property's data can be populated with a file's contents
as follows:

node {
	prop = /incbin/("path/to/data");
};

A subset of a file can be included by passing start and size parameters.
For example, to include bytes 8 through 23:

node {
	prop = /incbin/("path/to/data", 8, 16);
};

As with /include/, non-absolute paths are looked for in the directory
of the source file that includes them.

Signed-off-by: Scott Wood <redacted>
---
Now that there's no imminent release tagging, can we merge this, or are
there any objections?

 Makefile     |    2 +-
 data.c       |   37 ++++++++++++++++++++++++++++++++++++-
 dtc-lexer.l  |    7 +++++++
 dtc-parser.y |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h        |    5 +++++
 5 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 8a47c34..d4d935c 100644
--- a/Makefile
+++ b/Makefile

@@ -16,7 +16,7 @@ LOCAL_VERSION =
 CONFIG_LOCALVERSION =
 
 CPPFLAGS = -I libfdt
-CFLAGS = -Wall -g -Os
+CFLAGS = -Wall -g -Os -D_FILE_OFFSET_BITS=64
 
 BISON = bison
 LEX = flex

diff --git a/data.c b/data.c
index a94718c..f9464bf 100644
--- a/data.c
+++ b/data.c

@@ -19,6 +19,7 @@
  */
 
 #include "dtc.h"
+#include "srcpos.h"
 
 void data_free(struct data d)
 {

@@ -189,7 +190,41 @@ struct data data_copy_file(FILE *f, size_t len)
 	d = data_grow_for(empty_data, len);
 
 	d.len = len;
-	fread(d.val, len, 1, f);
+	if (fread(d.val, len, 1, f) != 1) {
+		yyerrorf("Couldn't read %zu bytes from file: %s",
+		         len, feof(f) ? "end-of-file" : strerror(errno));
+		return empty_data;
+	}
+
+	return d;
+}
+
+struct data data_copy_file_all(FILE *f)
+{
+	char buf[4096];
+	struct data d = empty_data;
+
+	while (1) {
+		size_t ret = fread(buf, 1, sizeof(buf), f);
+		if (ret == 0) {
+			if (!feof(f))
+				yyerrorf("Error reading file: %s", strerror(errno));
+
+			break;
+		}
+
+		assert(ret <= sizeof(buf));
+
+		d = data_grow_for(d, ret);
+		memcpy(d.val + d.len, buf, ret);
+
+		if (d.len + ret < d.len) {
+			yyerror("Binary include too large");
+			break;
+		}
+
+		d.len += ret;
+	}
 
 	return d;
 }

diff --git a/dtc-lexer.l b/dtc-lexer.l
index 920b87f..ee32308 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l

@@ -190,6 +190,13 @@ static int dts_version; /* = 0 */
 			return DT_PROPNODENAME;
 		}
 
+"/incbin/"	{
+			yylloc.file = srcpos_file;
+			yylloc.first_line = yylineno;
+			DPRINT("Binary Include\n");
+			return DT_INCBIN;
+		}
+
 <*>[[:space:]]+	/* eat whitespace */
 
 <*>"/*"([^*]|\*+[^*/])*\*+"/"	{

diff --git a/dtc-parser.y b/dtc-parser.y
index bae3c32..3a24d14 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y

@@ -21,6 +21,8 @@
 %locations
 
 %{
+#include <stdio.h>
+
 #include "dtc.h"
 #include "srcpos.h"
 

@@ -47,6 +49,7 @@ extern int treesource_error;
 	struct node *node;
 	struct node *nodelist;
 	struct reserve_info *re;
+	struct range range;
 }
 
 %token DT_V1

@@ -59,6 +62,7 @@ extern int treesource_error;
 %token <data> DT_STRING
 %token <labelref> DT_LABEL
 %token <labelref> DT_REF
+%token DT_INCBIN
 
 %type <data> propdata
 %type <data> propdataprefix

@@ -79,6 +83,7 @@ extern int treesource_error;
 %type <node> subnode
 %type <nodelist> subnodes
 %type <labelref> label
+%type <range> mayberange
 
 %%
 

@@ -197,12 +202,56 @@ propdata:
 		{
 			$$ = data_add_marker($1, REF_PATH, $2);
 		}
+	| propdataprefix DT_INCBIN '(' DT_STRING mayberange ')'
+		{
+			struct search_path path = { srcpos_file->dir, NULL, NULL };
+			struct dtc_file *file = dtc_open_file($4.val, &path);
+
+			if (!file) {
+				yyerrorf("Cannot open file \"%s\": %s",
+				         $4.val, strerror(errno));
+			} else {
+				struct data d = empty_data;
+
+				if ($5.len >= 0) {
+					if (fseek(file->file, $5.start, SEEK_SET) == 0)
+						d = data_copy_file(file->file, $5.len);
+					else
+						yyerrorf("Couldn't seek to offset %llu in \"%s\": %s",
+						         (unsigned long long)$5.start,
+						         $4.val, strerror(errno));
+				} else {
+					d = data_copy_file_all(file->file);
+				}
+
+				$$ = data_merge($1, d);
+				dtc_close_file(file);
+			}
+		}
 	| propdata DT_LABEL
 		{
 			$$ = data_add_marker($1, LABEL, $2);
 		}
 	;
 
+mayberange:
+	  /* empty */
+		{
+			$$.len = -1;
+		}
+	| ',' addr ',' addr
+		{
+			$$.start = $2;
+			$$.len = $4;
+
+			if ($$.len != $4) {
+				yyerrorf("Length %llu is too large",
+				         (unsigned long long)$4);
+				$$.len = -1;
+			}
+		}
+	;
+
 propdataprefix:
 	  /* empty */
 		{

diff --git a/dtc.h b/dtc.h
index cba9d28..509fbc9 100644
--- a/dtc.h
+++ b/dtc.h

@@ -122,6 +122,10 @@ struct data {
 	struct marker *markers;
 };
 
+struct range {
+	off_t start;
+	int len;
+};
 
 #define empty_data ((struct data){ /* all .members = 0 or NULL */ })
 

@@ -138,6 +142,7 @@ struct data data_grow_for(struct data d, int xlen);
 struct data data_copy_mem(const char *mem, int len);
 struct data data_copy_escape_string(const char *s, int len);
 struct data data_copy_file(FILE *f, size_t len);
+struct data data_copy_file_all(FILE *f);
 
 struct data data_append_data(struct data d, const void *p, int len);
 struct data data_insert_at_marker(struct data d, struct marker *m,

-- 
1.5.3

On Wed, Feb 20, 2008 at 01:19:41PM -0600, Scott Wood wrote:

A property's data can be populated with a file's contents
as follows:

node {
	prop = /incbin/("path/to/data");
};

A subset of a file can be included by passing start and size parameters.
For example, to include bytes 8 through 23:

node {
	prop = /incbin/("path/to/data", 8, 16);
};

As with /include/, non-absolute paths are looked for in the directory
of the source file that includes them.

I still dislike the syntax, but haven't thought of a better one yet.
There are some issues with the implementation too, but I've been a bit
too busy with ePAPR stuff to review properly.

Soon, I hope.

-- 
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

On Wed, Feb 20, 2008 at 12:19 PM, Scott Wood [off-list ref] wrote:

A property's data can be populated with a file's contents
 as follows:

 node {
        prop = /incbin/("path/to/data");
 };

 A subset of a file can be included by passing start and size parameters.
 For example, to include bytes 8 through 23:

 node {
        prop = /incbin/("path/to/data", 8, 16);
 };

Can I ask; what is the intended usage of such a thing?  How large
would a typical binary blob be?

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

On Thu, 2008-02-21 at 23:05 -0700, Grant Likely wrote:

Can I ask; what is the intended usage of such a thing?  How large
would a typical binary blob be?

Device firmware?

-- 
dwmw2

Grant Likely wrote:

On Wed, Feb 20, 2008 at 12:19 PM, Scott Wood [off-list ref] wrote:

quoted

A property's data can be populated with a file's contents
 as follows:

 node {
        prop = /incbin/("path/to/data");
 };

 A subset of a file can be included by passing start and size parameters.
 For example, to include bytes 8 through 23:

 node {
        prop = /incbin/("path/to/data", 8, 16);
 };

Can I ask; what is the intended usage of such a thing?  How large
would a typical binary blob be?

Binary includes are a critical feature of the new image format for
U-Boot. In short: we're describing image's content and other data
(architecture, os, image type, etc) in dts format, and then use dtc to
generate the file that is transferred to the target and booted by
U-Boot. Excerpt from an actual file describing an image:

/{
         images {
                 kernel@1 {
                         data = /incbin/("./vmlinux.bin.gz");
                         type = "kernel";
                         arch = "ppc";
                         os = "linux";
                         compression = "gzip";
                         load = <00000000>;
                         entry = <00000000>;
                 };
         };
};

As to the size of the final blob in our case, it depends. It seems that
in case of a powerpc Linux booting scenario, most of the times the size
will be roughly the size of a compressed kernel + the size of the FDT
blob, eventually plus the size of a ramdisk. Note however, that the new
image format has provisions for handling multiple components of the same
type in one image, so for example you could have three kernels, two
ramdisks and a couple of fdt blobs in one image, and the actual booting
configuration could be selected in U-Boot. In such a case, the final
blob can be arbitrarily large.

Regards,
Bartlomiej

On Fri, Feb 22, 2008 at 2:02 AM, David Woodhouse [off-list ref] wrote:

 On Thu, 2008-02-21 at 23:05 -0700, Grant Likely wrote:
 > Can I ask; what is the intended usage of such a thing?  How large
 > would a typical binary blob be?

 Device firmware?

That's what I was wondering about.  Is this really a good idea?
Effectively, in the Linux kernel the device tree is being used as a
big blob of initdata, except that it cannot free the memory allocated
by the device tree when it is complete.  As it is right now, device
trees seem to weigh in between 4-8kB, a pretty insignificant size for
memory that cannot be reclaimed.  However, if firmware blobs start
getting added to the tree that size could balloon pretty quickly.

I'm not actually opposed to the feature, and I understand that it is
needed for the new u-boot image format, but I think we need some usage
guidelines in place when this feature is merged.  As much as possible
I think we should be directing developers to use the existing firmware
loading facilities in the Linux kernel.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

On Thu, Feb 21, 2008 at 11:05:58PM -0700, Grant Likely wrote:

On Wed, Feb 20, 2008 at 12:19 PM, Scott Wood [off-list ref] wrote:

quoted

A property's data can be populated with a file's contents
 as follows:

 node {
        prop = /incbin/("path/to/data");
 };

 A subset of a file can be included by passing start and size parameters.
 For example, to include bytes 8 through 23:

 node {
        prop = /incbin/("path/to/data", 8, 16);
 };

Can I ask; what is the intended usage of such a thing?  How large
would a typical binary blob be?

I use it for embedding guest device trees in a hypervisor's device tree.

-Scott

David Gibson wrote:

quoted

node {
	prop = /incbin/("path/to/data");
};

node {
	prop = /incbin/("path/to/data", 8, 16);
};

I still dislike the syntax, but haven't thought of a better one yet.
There are some issues with the implementation too, but I've been a bit
too busy with ePAPR stuff to review properly.

I'm OK with the syntax, but whatever-ish.

Would these be better?:

    prop = /call/(incbin, "path/to/data", 17, 23);
    prop = /call[incbin]/("path/to/data");
    prop = /call incbin/("path/to/data", 12, 12+10);

What is the aspect of the syntax that you don't like?
I think we essentially need to stick in the /.../ realm
to be consistent with the other non-standard names being
used, like /include/.

I can see a generalized form that allows other pre-defined
or user-defined "functions" to be introduced and called
or used in a similar way:

    prop = <(22 + /fibonacci/(7)) 1000>;

    prop = /directoryof/("/path/to/some/file.doc");

    interrupt-map = /pci_int_map/(8000, 2, 14);

or whatever.  We can paint this bikeshed for a long time
if we need to.  Or, we can get down to some serious issue
if there are any.  Are there?

jdl

On Fri, Feb 22, 2008 at 12:12:25PM -0600, Jon Loeliger wrote:

David Gibson wrote:

quoted

quoted

node {
	prop = /incbin/("path/to/data");
};

node {
	prop = /incbin/("path/to/data", 8, 16);
};

I still dislike the syntax, but haven't thought of a better one yet.
There are some issues with the implementation too, but I've been a bit
too busy with ePAPR stuff to review properly.

I'm OK with the syntax, but whatever-ish.

Would these be better?:

    prop = /call/(incbin, "path/to/data", 17, 23);
    prop = /call[incbin]/("path/to/data");
    prop = /call incbin/("path/to/data", 12, 12+10);

Ugh, I like those even less.

What is the aspect of the syntax that you don't like?

Well, when I first objected, I didn't really know why.  But
contemplating your discussion below has helped by crystallise why this
syntax is making my ugly sense tingle.

1) I've no problem with using /word/ for "reserved words".  It's what
we're already doing, and importantly it's lexically distinct in all
contexts, even in proximity to the lexically troublesome node and
property names.

2) I've no problem with <something>(arg, arg, arg) function syntax.
Functions are handy, and this is consistent with our "least suprise to
C programmers" design principle.

What I don't like is the combination of the two.  Using the /word/
form in (1) suggests that each /word/ is a lexically distinct symbol
with functions in different contexts: consider /dts-v1/, /include/,
/memreserve/ - they're all used only in their own distinct context.
Use of /word/s in (2) would suggest that each /word/ is just an
identifier for a different function, and should all be usable in a
similar grammtical context - which won't be true of /memreserve/,
/dts-v1/ and any other truly lexically distinct symbols we need to
add.

I think we essentially need to stick in the /.../ realm
to be consistent with the other non-standard names being
used, like /include/.

So, with the thoughts about, I come to the conclusion that, yes, we
should stick to /.../ for things which really act like reserved words,
but binary includes aren't one of those things.

Unlike source includes, binary includes only make sense inside a
property *value* (or to put it another way, an a context suitable for
an expression).  Which means the possible lexical confusion with
node/property names that motivated the /foo/ form in the first place
disappears.


So, I see two good options for the binary include.  Once is to treat
binary includes as a new special operator.  That operator (as it's
own, lexically distinct symbol) can be represented by a /word/, but in
that case, using it shouldn't look like a general function call.

Or, we can treat binary includes as a built-in function, see below.

I can see a generalized form that allows other pre-defined
or user-defined "functions" to be introduced and called
or used in a similar way:

    prop = <(22 + /fibonacci/(7)) 1000>;

    prop = /directoryof/("/path/to/some/file.doc");

    interrupt-map = /pci_int_map/(8000, 2, 14);

or whatever.  We can paint this bikeshed for a long time
if we need to.  Or, we can get down to some serious issue
if there are any.  Are there?

So, I like the notion of functions like this, but with identifiers
that aren't /word/s.  Re-invoking the "least surprise to C
programmers" principle, in general I think the identifiers should be
as C identifiers (i.e. [a-zA-Z_][a-zA-Z0-9_]*).  With one caveat, it's
not essential but it might be worthwhile to make built-in function
identifiers obviously distinct from user-defined ones (if we add those
in future).


PS. I do see one potential gotcha with C-like function syntax.  As we
add expression support, the obvious way to handle constructs like:
	prop = <0x1 0x2>, "string";
Is to treat ',' as a bytestring concatenation operator.  There's
potentialy ambiguity between that usage and it's use as a separator
for function arguments.  Of course the same issue exists in C with its
comma operator, so it's not a showstopper.

-- 
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

On Wed, Feb 20, 2008 at 01:19:41PM -0600, Scott Wood wrote:

A property's data can be populated with a file's contents
as follows:

node {
	prop = /incbin/("path/to/data");
};

A subset of a file can be included by passing start and size parameters.
For example, to include bytes 8 through 23:

node {
	prop = /incbin/("path/to/data", 8, 16);
};

As with /include/, non-absolute paths are looked for in the directory
of the source file that includes them.

Well, while I discuss the syntax with Jon, here's some comments on the
implementation.

Is this portable?

Ugh.
	1) Error messages direct to the user really don't belong in
data.c's low-level support routines.  This does mean we might need to
substantially change this function so there's some other way of
reporting the error to the caller who can report it.
	2) The horrid name 'yyerror()' exists only to match what bison
expects.  It should never be used outside the parse code (we probably
should create more generally usable error/warning functions, though).
	3) Is %zu portable?

+	}
+
+	return d;
+}
+
+struct data data_copy_file_all(FILE *f)

It should be possible to combine these two functions.

+{
+	char buf[4096];
+	struct data d = empty_data;
+
+	while (1) {
+		size_t ret = fread(buf, 1, sizeof(buf), f);
+		if (ret == 0) {
+			if (!feof(f))
+				yyerrorf("Error reading file: %s", strerror(errno));
+
+			break;
+		}
+
+		assert(ret <= sizeof(buf));
+
+		d = data_grow_for(d, ret);
+		memcpy(d.val + d.len, buf, ret);
+
+		if (d.len + ret < d.len) {
+			yyerror("Binary include too large");

Hrm.  A test which will cover this case should go into
data_grow_for(), I think.

-- 
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

On Tue, Feb 26, 2008 at 11:39:55AM +1100, David Gibson wrote:

quoted

diff --git a/Makefile b/Makefile
index 8a47c34..d4d935c 100644
--- a/Makefile
+++ b/Makefile

@@ -16,7 +16,7 @@ LOCAL_VERSION =
 CONFIG_LOCALVERSION =
 
 CPPFLAGS = -I libfdt
-CFLAGS = -Wall -g -Os
+CFLAGS = -Wall -g -Os -D_FILE_OFFSET_BITS=64

Is this portable?

It seems fairly widespread, but I don't know if it's specified by any
standards.  Some googling shows that _LARGE_FILES and _LARGEFILE64_SOURCE
are used in some environments.

quoted

 	d.len = len;
-	fread(d.val, len, 1, f);
+	if (fread(d.val, len, 1, f) != 1) {
+		yyerrorf("Couldn't read %zu bytes from file: %s",
+		         len, feof(f) ? "end-of-file" : strerror(errno));
+		return empty_data;

Ugh.
	1) Error messages direct to the user really don't belong in
data.c's low-level support routines.  This does mean we might need to
substantially change this function so there's some other way of
reporting the error to the caller who can report it.

Any suggestions on a mechanism that doesn't throw away information by
condensing everything down to a single error number?

	2) The horrid name 'yyerror()' exists only to match what bison
expects.  It should never be used outside the parse code (we probably
should create more generally usable error/warning functions, though).

Fine, so suggest a name.

	3) Is %zu portable?

I believe it's part of C99.

quoted

+	}
+
+	return d;
+}
+
+struct data data_copy_file_all(FILE *f)

It should be possible to combine these two functions.

Probably.

quoted

+		d = data_grow_for(d, ret);
+		memcpy(d.val + d.len, buf, ret);
+
+		if (d.len + ret < d.len) {
+			yyerror("Binary include too large");

Hrm.  A test which will cover this case should go into
data_grow_for(), I think.

Agreed.

-Scott

On Feb 25, 2008, at 6:39 PM, David Gibson wrote:

On Wed, Feb 20, 2008 at 01:19:41PM -0600, Scott Wood wrote:

quoted

A property's data can be populated with a file's contents
as follows:

node {
	prop = /incbin/("path/to/data");
};

A subset of a file can be included by passing start and size  
parameters.
For example, to include bytes 8 through 23:

node {
	prop = /incbin/("path/to/data", 8, 16);
};

As with /include/, non-absolute paths are looked for in the directory
of the source file that includes them.

Well, while I discuss the syntax with Jon, here's some comments on the
implementation.

have we made any progress on the syntax?

- k

Kumar Gala wrote:

On Feb 25, 2008, at 6:39 PM, David Gibson wrote:

quoted

On Wed, Feb 20, 2008 at 01:19:41PM -0600, Scott Wood wrote:

quoted

A property's data can be populated with a file's contents
as follows:

node {
    prop = /incbin/("path/to/data");
};

A subset of a file can be included by passing start and size parameters.
For example, to include bytes 8 through 23:

node {
    prop = /incbin/("path/to/data", 8, 16);
};

As with /include/, non-absolute paths are looked for in the directory
of the source file that includes them.

That issue was resolved, I believe.

quoted

Well, while I discuss the syntax with Jon, here's some comments on the
implementation.

have we made any progress on the syntax?

My last suggestions garnered a "I like that even less." response,
so as far as I know, we're still waiting for a good proposal here.

jdl

On Tue, May 27, 2008 at 12:59:32PM -0500, Jon Loeliger wrote:

Kumar Gala wrote:

quoted

On Feb 25, 2008, at 6:39 PM, David Gibson wrote:

quoted

On Wed, Feb 20, 2008 at 01:19:41PM -0600, Scott Wood wrote:

quoted

A property's data can be populated with a file's contents
as follows:

node {
    prop = /incbin/("path/to/data");
};

A subset of a file can be included by passing start and size parameters.
For example, to include bytes 8 through 23:

node {
    prop = /incbin/("path/to/data", 8, 16);
};

As with /include/, non-absolute paths are looked for in the directory
of the source file that includes them.

That issue was resolved, I believe.

quoted

quoted

Well, while I discuss the syntax with Jon, here's some comments on the
implementation.

have we made any progress on the syntax?

My last suggestions garnered a "I like that even less." response,
so as far as I know, we're still waiting for a good proposal here.

A while back I sent out a spiel explaining more clearly why I didn't
like it, and where I thought we should go with this, but I don't think
anyone noticed it at the time.  I'll resend.

I started working towards a version of this I liked, but was
sidetracked by a combination of my own other work, and the fact that
Jon's been busy meaning there's a rather large lag on merging dtc
patches.

-- 
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

On Thu, May 29, 2008 at 09:58:59AM +1000, David Gibson wrote:

On Tue, May 27, 2008 at 12:59:32PM -0500, Jon Loeliger wrote:

quoted

Kumar Gala wrote:

quoted

On Feb 25, 2008, at 6:39 PM, David Gibson wrote:

quoted

On Wed, Feb 20, 2008 at 01:19:41PM -0600, Scott Wood wrote:

quoted

A property's data can be populated with a file's contents
as follows:

node {
    prop = /incbin/("path/to/data");
};

A subset of a file can be included by passing start and size parameters.
For example, to include bytes 8 through 23:

node {
    prop = /incbin/("path/to/data", 8, 16);
};

As with /include/, non-absolute paths are looked for in the directory
of the source file that includes them.

That issue was resolved, I believe.

quoted

quoted

Well, while I discuss the syntax with Jon, here's some comments on the
implementation.

have we made any progress on the syntax?

My last suggestions garnered a "I like that even less." response,
so as far as I know, we're still waiting for a good proposal here.

A while back I sent out a spiel explaining more clearly why I didn't
like it, and where I thought we should go with this, but I don't think
anyone noticed it at the time.  I'll resend.

See
http://www.nabble.com/Re%3A--PATCH--Add-support-for-binary-includes.-p16149085.html

-- 
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

David Gibson wrote:

A while back I sent out a spiel explaining more clearly why I didn't
like it, and where I thought we should go with this, but I don't think
anyone noticed it at the time.  I'll resend.

Thanks.

I started working towards a version of this I liked, but was
sidetracked by a combination of my own other work, and the fact that
Jon's been busy meaning there's a rather large lag on merging dtc
patches.

I believe I am fully caught up at this point.

jdl

On Thu, May 29, 2008 at 09:04:29AM -0500, Jon Loeliger wrote:

David Gibson wrote:

quoted

A while back I sent out a spiel explaining more clearly why I didn't
like it, and where I thought we should go with this, but I don't think
anyone noticed it at the time.  I'll resend.

Thanks.

Actually realised that spiel was in the same mail as by "I like this
even less" comment - same one I sent a pointer to an archive for.  I'd
been hoping for some kind of response from you.

quoted

I started working towards a version of this I liked, but was
sidetracked by a combination of my own other work, and the fact that
Jon's been busy meaning there's a rather large lag on merging dtc
patches.

I believe I am fully caught up at this point.

Not quite, this one slipped through the cracks:

dtc: Fix some printf() format warnings when compiling 64-bit

Currently, dtc generates a few gcc build warnings if built for a
64-bit target, due to the altered type of uint64_t and size_t.  This
patch fixes the warnings (without generating new warnings for 32-bit).

Signed-off-by: David Gibson <redacted>

Index: dtc/tests/dtbs_equal_ordered.c
===================================================================

--- dtc.orig/tests/dtbs_equal_ordered.c	2008-05-20 13:06:50.000000000 +1000
+++ dtc/tests/dtbs_equal_ordered.c	2008-05-20 13:07:41.000000000 +1000

@@ -49,7 +49,10 @@ void compare_mem_rsv(const void *fdt1, c
 		if ((addr1 != addr2) || (size1 != size2))
 			FAIL("Mismatch in reserve entry %d: "
 			     "(0x%llx, 0x%llx) != (0x%llx, 0x%llx)", i,
-			     addr1, size1, addr2, size2);
+			     (unsigned long long)addr1,
+			     (unsigned long long)size1,
+			     (unsigned long long)addr2,
+			     (unsigned long long)size2);
 	}
 }
 

Index: dtc/tests/references.c
===================================================================

--- dtc.orig/tests/references.c	2008-05-20 13:05:21.000000000 +1000
+++ dtc/tests/references.c	2008-05-20 13:05:53.000000000 +1000

@@ -38,7 +38,7 @@ void check_ref(const void *fdt, int node
 	if (!p)
 		FAIL("fdt_getprop(%d, \"ref\"): %s", node, fdt_strerror(len));
 	if (len != sizeof(*p))
-		FAIL("'ref' in node at %d has wrong size (%d instead of %d)",
+		FAIL("'ref' in node at %d has wrong size (%d instead of %zd)",
 		     node, len, sizeof(*p));
 	ref = fdt32_to_cpu(*p);
 	if (ref != checkref)

@@ -49,7 +49,7 @@ void check_ref(const void *fdt, int node
 	if (!p)
 		FAIL("fdt_getprop(%d, \"lref\"): %s", node, fdt_strerror(len));
 	if (len != sizeof(*p))
-		FAIL("'lref' in node at %d has wrong size (%d instead of %d)",
+		FAIL("'lref' in node at %d has wrong size (%d instead of %zd)",
 		     node, len, sizeof(*p));
 	ref = fdt32_to_cpu(*p);
 	if (ref != checkref)

-- 
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

David Gibson wrote:

What I don't like is the combination of the two.  Using the /word/
form in (1) suggests that each /word/ is a lexically distinct symbol
with functions in different contexts: consider /dts-v1/, /include/,
/memreserve/ - they're all used only in their own distinct context.
Use of /word/s in (2) would suggest that each /word/ is just an
identifier for a different function, and should all be usable in a
similar grammtical context - which won't be true of /memreserve/,
/dts-v1/ and any other truly lexically distinct symbols we need to
add.

I don't understand this conclusion -- I wouldn't expect to be able to 
use "for" or "while" at file scope of C code, just because I can use 
"struct", "int", or "sizeof" there.  The slashes are simply a way of 
creating reserved words, some of which happen to be function-like.

So, I like the notion of functions like this, but with identifiers
that aren't /word/s.  Re-invoking the "least surprise to C
programmers" principle, in general I think the identifiers should be
as C identifiers (i.e. [a-zA-Z_][a-zA-Z0-9_]*).

That would make it difficult to have function-like syntax outside of 
properties.

 With one caveat, it's
not essential but it might be worthwhile to make built-in function
identifiers obviously distinct from user-defined ones (if we add those
in future).

We have a way to make them obviously distinct -- slashes. :-)

-Scott

On Thu, May 29, 2008 at 09:04:29AM -0500, Jon Loeliger wrote:

quoted

I believe I am fully caught up at this point.

Not quite, this one slipped through the cracks:

Rats.

dtc: Fix some printf() format warnings when compiling 64-bit

Currently, dtc generates a few gcc build warnings if built for a
64-bit target, due to the altered type of uint64_t and size_t.  This
patch fixes the warnings (without generating new warnings for 32-bit).

Signed-off-by: David Gibson <redacted>

Please use git for your patches.  Really.

Thanks,
jdl

On Fri, May 30, 2008 at 01:54:59PM -0500, Scott Wood wrote:

David Gibson wrote:

quoted

What I don't like is the combination of the two.  Using the /word/
form in (1) suggests that each /word/ is a lexically distinct symbol
with functions in different contexts: consider /dts-v1/, /include/,
/memreserve/ - they're all used only in their own distinct context.
Use of /word/s in (2) would suggest that each /word/ is just an
identifier for a different function, and should all be usable in a
similar grammtical context - which won't be true of /memreserve/,
/dts-v1/ and any other truly lexically distinct symbols we need to
add.

I don't understand this conclusion -- I wouldn't expect to be able to  
use "for" or "while" at file scope of C code, just because I can use  
"struct", "int", or "sizeof" there.  The slashes are simply a way of  
creating reserved words, some of which happen to be function-like.

Heh, when I started revisiting this after my long hiatus doing other
things, I was thinking the same way.  I still have a few misgivings,
but then the nice thing about the slash-delimited reserved word thing
is that even if we come up with a new, nicer syntax it's not going to
hurt to keep the slash-form around for compatibility.

sizeof is an interesting example.  As you point out it's an example of
a function-like reserved word, which given our existing approach to
reserved words supports your syntax.  On the other hand, we may well
want a sizeof operator in dtc itself as part of our expression
support, and in that case, the "be like C" principle suggests it
should be rendered as "sizeof" rather than "/sizeof/".

But as I said that can be dealt with in the future without breaking
compatibility.  Objection withdrawn.

Sorry it's taken this long :(.

quoted

So, I like the notion of functions like this, but with identifiers
that aren't /word/s.  Re-invoking the "least surprise to C
programmers" principle, in general I think the identifiers should be
as C identifiers (i.e. [a-zA-Z_][a-zA-Z0-9_]*).

That would make it difficult to have function-like syntax outside of  
properties.

Not really.  The only thing such identifiers are ambiguous with is
node and property names, and we've already headed down the path of
making lexical contexts in which node/property names are recognized be
the exception.

My plan for the future here was to have { } always be the thing that
introduces a node/property name context.  That's roughly right for the
existing node syntax - obviously there's a bit more complexity there,
since we switch back to normal/expression context after a node/prop
name, then back to node/prop name context after ';'.  Then for any
expression operators that take node/prop names, I suggest we also use
{ }, so if 'foo' is a node expression, then maybe 'foo{reg}' to
extract the 'reg' property from the node represented by foo.

I'll need a little finessing, but I think using a lexer context stack
it shouldn't be too hard to handle.

-- 
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

David Gibson wrote:

On Fri, May 30, 2008 at 01:54:59PM -0500, Scott Wood wrote:

quoted

David Gibson wrote:

quoted

What I don't like is the combination of the two.  Using the /word/
form in (1) suggests that each /word/ is a lexically distinct symbol
with functions in different contexts: consider /dts-v1/, /include/,
/memreserve/ - they're all used only in their own distinct context.
Use of /word/s in (2) would suggest that each /word/ is just an
identifier for a different function, and should all be usable in a
similar grammtical context - which won't be true of /memreserve/,
/dts-v1/ and any other truly lexically distinct symbols we need to
add.

I don't understand this conclusion -- I wouldn't expect to be able to  
use "for" or "while" at file scope of C code, just because I can use  
"struct", "int", or "sizeof" there.  The slashes are simply a way of  
creating reserved words, some of which happen to be function-like.

Heh, when I started revisiting this after my long hiatus doing other
things, I was thinking the same way.  I still have a few misgivings,
but then the nice thing about the slash-delimited reserved word thing
is that even if we come up with a new, nicer syntax it's not going to
hurt to keep the slash-form around for compatibility.

sizeof is an interesting example.  As you point out it's an example of
a function-like reserved word, which given our existing approach to
reserved words supports your syntax.  On the other hand, we may well
want a sizeof operator in dtc itself as part of our expression
support, and in that case, the "be like C" principle suggests it
should be rendered as "sizeof" rather than "/sizeof/".

But as I said that can be dealt with in the future without breaking
compatibility.  Objection withdrawn.

Hi,

To add one more point to the discussion: the /incbin/ syntax is being
used in the new image format of U-Boot (we're using dtc with original
patch by Scott Wood, i.e.,
http://www.nabble.com/-PATCH--Add-support-for-binary-includes.-td15596760.html).

If possible, it would be good to have the original syntax preserved once
the feature is merged into the mainline dtc. BTW: any idea on when this
might happen?

Regards,
Bartlomiej

David Gibson wrote:

But as I said that can be dealt with in the future without breaking
compatibility.  Objection withdrawn.

And on that note, I officially implore Scott to
re-submit his binary include patch!

Sorry it's taken this long :(.

No problem; no apology needed. [*1*]

jdl


[*1*] -- Or would this have been better?::-)
      <Darth Vader Voice> Apology...accepted. </Darth Vader Voice>

On Wed, 04 Jun 2008 09:26:23 -0500
Jon Loeliger [off-list ref] wrote:

David Gibson wrote:

quoted

But as I said that can be dealt with in the future without breaking
compatibility.  Objection withdrawn.

And on that note, I officially implore Scott to
re-submit his binary include patch!

quoted

Sorry it's taken this long :(.

No problem; no apology needed. [*1*]

jdl


[*1*] -- Or would this have been better?::-)
      <Darth Vader Voice> Apology...accepted. </Darth Vader Voice>

If you had only used:

<Darth Vader Voice>
I find your lack of faith... disturbing.
</Darth Vader Voice>

when the objection was first raised, this whole thread could have been
avoided.

josh

On Wed, Jun 04, 2008 at 09:26:23AM -0500, Jon Loeliger wrote:

David Gibson wrote:

quoted

But as I said that can be dealt with in the future without breaking
compatibility.  Objection withdrawn.

And on that note, I officially implore Scott to
re-submit his binary include patch!

Scott's original patch does still have some implementation details I
didn't like.  So in the interests of saving time, I've addressed some
of those, added a testcase, and and now resubmitting my revised
version of Scott's patch.

dtc: Add support for binary includes.

A property's data can be populated with a file's contents
as follows:

node {
	prop = /incbin/("path/to/data");
};

A subset of a file can be included by passing start and size parameters.
For example, to include bytes 8 through 23:

node {
	prop = /incbin/("path/to/data", 8, 16);
};

As with /include/, non-absolute paths are looked for in the directory
of the source file that includes them.

Implementation revised, and a testcase added by David Gibson

Signed-off-by: Scott Wood <redacted>
Signed-off-by: David Gibson <redacted>

Index: dtc/data.c
===================================================================

--- dtc.orig/data.c	2008-06-11 11:50:29.000000000 +1000
+++ dtc/data.c	2008-06-11 11:50:38.000000000 +1000

@@ -167,14 +167,29 @@
 	return d;
 }
 
-struct data data_copy_file(FILE *f, size_t len)
+struct data data_copy_file(FILE *f, size_t maxlen)
 {
-	struct data d;
+	struct data d = empty_data;
+
+	while (!feof(f) && (d.len < maxlen)) {
+		size_t chunksize, ret;
+
+		if (maxlen == -1)
+			chunksize = 4096;
+		else
+			chunksize = maxlen - d.len;
+
+		d = data_grow_for(d, chunksize);
+		ret = fread(d.val + d.len, 1, chunksize, f);
+
+		if (ferror(f))
+			die("Error reading file into data: %s", strerror(errno));
 
-	d = data_grow_for(empty_data, len);
+		if (d.len + ret < d.len)
+			die("Overflow reading file into data\n");
 
-	d.len = len;
-	fread(d.val, len, 1, f);
+		d.len += ret;
+	}
 
 	return d;
 }

Index: dtc/dtc-lexer.l
===================================================================

--- dtc.orig/dtc-lexer.l	2008-06-11 11:50:29.000000000 +1000
+++ dtc/dtc-lexer.l	2008-06-11 11:50:38.000000000 +1000

@@ -190,6 +190,13 @@
 			return DT_PROPNODENAME;
 		}
 
+"/incbin/"	{
+			yylloc.file = srcpos_file;
+			yylloc.first_line = yylineno;
+			DPRINT("Binary Include\n");
+			return DT_INCBIN;
+		}
+
 <*>[[:space:]]+	/* eat whitespace */
 
 <*>"/*"([^*]|\*+[^*/])*\*+"/"	{

Index: dtc/dtc-parser.y
===================================================================

--- dtc.orig/dtc-parser.y	2008-06-11 11:50:29.000000000 +1000
+++ dtc/dtc-parser.y	2008-06-11 11:50:38.000000000 +1000

@@ -21,6 +21,8 @@
 %locations
 
 %{
+#include <stdio.h>
+
 #include "dtc.h"
 #include "srcpos.h"
 

@@ -59,6 +61,7 @@
 %token <data> DT_STRING
 %token <labelref> DT_LABEL
 %token <labelref> DT_REF
+%token DT_INCBIN
 
 %type <data> propdata
 %type <data> propdataprefix

@@ -197,6 +200,34 @@
 		{
 			$$ = data_add_marker($1, REF_PATH, $2);
 		}
+	| propdataprefix DT_INCBIN '(' DT_STRING ',' addr ',' addr ')'
+		{
+			struct search_path path = { srcpos_file->dir, NULL, NULL };
+			struct dtc_file *file = dtc_open_file($4.val, &path);
+			struct data d = empty_data;
+
+			if ($6 != 0)
+				if (fseek(file->file, $6, SEEK_SET) != 0)
+					yyerrorf("Couldn't seek to offset %llu in \"%s\": %s",
+						 (unsigned long long)$6,
+						 $4.val, strerror(errno));
+
+			d = data_copy_file(file->file, $8);
+
+			$$ = data_merge($1, d);
+			dtc_close_file(file);
+		}
+	| propdataprefix DT_INCBIN '(' DT_STRING ')'
+		{
+			struct search_path path = { srcpos_file->dir, NULL, NULL };
+			struct dtc_file *file = dtc_open_file($4.val, &path);
+			struct data d = empty_data;
+
+			d = data_copy_file(file->file, -1);
+
+			$$ = data_merge($1, d);
+			dtc_close_file(file);
+		}
 	| propdata DT_LABEL
 		{
 			$$ = data_add_marker($1, LABEL, $2);

Index: dtc/tests/incbin.bin
===================================================================

--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ dtc/tests/incbin.bin	2008-06-11 11:50:38.000000000 +1000

@@ -0,0 +1 @@
+abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ

\ No newline at end of file
Index: dtc/tests/incbin.c
===================================================================

--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ dtc/tests/incbin.c	2008-06-11 11:50:38.000000000 +1000

@@ -0,0 +1,75 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for string escapes in dtc
+ * Copyright (C) 2006 David Gibson, IBM Corporation.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <errno.h>
+
+#include <fdt.h>
+#include <libfdt.h>
+
+#include "tests.h"
+#include "testdata.h"
+
+#define CHUNKSIZE	1024
+
+void *load_file(const char *name, int *len)
+{
+	FILE *f;
+	void *buf = NULL;
+	int bufsize = 0, n;
+
+	*len = 0;
+
+	f = fopen(name, "r");
+	if (!f)
+		FAIL("Couldn't open \"%s\": %s", name, strerror(errno));
+
+	while (!feof(f)) {
+		if (bufsize < (*len + CHUNKSIZE)) {
+			buf = xrealloc(buf, *len + CHUNKSIZE);
+			bufsize = *len + CHUNKSIZE;
+		}
+
+		n = fread(buf + *len, 1, CHUNKSIZE, f);
+		if (ferror(f))
+			FAIL("Error reading \"%s\": %s", name, strerror(errno));
+		*len += n;
+	}
+
+	return buf;
+}
+
+int main(int argc, char *argv[])
+{
+	void *fdt, *incbin;
+	int len;
+
+	test_init(argc, argv);
+
+	incbin = load_file("incbin.bin", &len);
+	fdt = load_blob_arg(argc, argv);
+
+	check_getprop(fdt, 0, "incbin", len, incbin);
+	check_getprop(fdt, 0, "incbin-partial", 17, incbin + 13);
+
+	PASS();
+}

Index: dtc/tests/incbin.dts
===================================================================

--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ dtc/tests/incbin.dts	2008-06-11 11:50:38.000000000 +1000

@@ -0,0 +1,6 @@
+/dts-v1/;
+
+/ {
+	incbin = /incbin/("incbin.bin");
+	incbin-partial = /incbin/("incbin.bin", 13, 17);
+};

Index: dtc/tests/Makefile.tests
===================================================================

--- dtc.orig/tests/Makefile.tests	2008-06-11 11:50:29.000000000 +1000
+++ dtc/tests/Makefile.tests	2008-06-11 11:50:38.000000000 +1000

@@ -9,7 +9,7 @@
 	sw_tree1 \
 	move_and_save mangle-layout nopulate \
 	open_pack rw_tree1 set_name setprop del_property del_node \
-	string_escapes references path-references boot-cpuid \
+	string_escapes references path-references boot-cpuid incbin \
 	dtbs_equal_ordered \
 	add_subnode_with_nops
 LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)

Index: dtc/tests/run_tests.sh
===================================================================

--- dtc.orig/tests/run_tests.sh	2008-06-11 11:50:38.000000000 +1000
+++ dtc/tests/run_tests.sh	2008-06-11 11:50:38.000000000 +1000

@@ -207,6 +207,10 @@
     run_dtc_test -I dts -O dtb -o dtc_comments-cmp.test.dtb comments-cmp.dts
     run_test dtbs_equal_ordered dtc_comments.test.dtb dtc_comments-cmp.test.dtb
 
+    # Check /incbin/ directive
+    run_dtc_test -I dts -O dtb -o incbin.test.dtb incbin.dts
+    run_test incbin incbin.test.dtb
+
     # Check boot_cpuid_phys handling
     run_dtc_test -I dts -O dtb -b 17 -o boot_cpuid.test.dtb empty.dts
     run_test boot-cpuid boot_cpuid.test.dtb 17

-- 
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

On Wed, Jun 11, 2008 at 11:58:39AM +1000, David Gibson wrote:

Scott's original patch does still have some implementation details I
didn't like.  So in the interests of saving time, I've addressed some
of those, added a testcase, and and now resubmitting my revised
version of Scott's patch.

Acked-by: Scott Wood <redacted>

-struct data data_copy_file(FILE *f, size_t len)
+struct data data_copy_file(FILE *f, size_t maxlen)
 {
-	struct data d;
+	struct data d = empty_data;
+
+	while (!feof(f) && (d.len < maxlen)) {
+		size_t chunksize, ret;
+
+		if (maxlen == -1)
+			chunksize = 4096;
+		else
+			chunksize = maxlen - d.len;
+
+		d = data_grow_for(d, chunksize);
+		ret = fread(d.val + d.len, 1, chunksize, f);
+
+		if (ferror(f))
+			die("Error reading file into data: %s", strerror(errno));

It'd be nice if we could keep the filename around for reporting here...

-Scott

On Thu, Jun 12, 2008 at 11:43:22AM -0500, Scott Wood wrote:

On Wed, Jun 11, 2008 at 11:58:39AM +1000, David Gibson wrote:

quoted

Scott's original patch does still have some implementation details I
didn't like.  So in the interests of saving time, I've addressed some
of those, added a testcase, and and now resubmitting my revised
version of Scott's patch.

Acked-by: Scott Wood <redacted>

quoted

-struct data data_copy_file(FILE *f, size_t len)
+struct data data_copy_file(FILE *f, size_t maxlen)
 {
-	struct data d;
+	struct data d = empty_data;
+
+	while (!feof(f) && (d.len < maxlen)) {
+		size_t chunksize, ret;
+
+		if (maxlen == -1)
+			chunksize = 4096;
+		else
+			chunksize = maxlen - d.len;
+
+		d = data_grow_for(d, chunksize);
+		ret = fread(d.val + d.len, 1, chunksize, f);
+
+		if (ferror(f))
+			die("Error reading file into data: %s", strerror(errno));

It'd be nice if we could keep the filename around for reporting here...

It would.  Well, I've been intending to clean up the input file
handling in several ways already, I'll see if that can be worked in.

-- 
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

David Gibson wrote:

quoted

It'd be nice if we could keep the filename around for reporting here...

It would.  Well, I've been intending to clean up the input file
handling in several ways already, I'll see if that can be worked in.

With Scott's ACK on this, I intend commit David's patch.
So if you adjust the input file reporting here, please
do it as an "additional patch."

And for the record, I will also make an official tagged
release just after any such cleanup patch appears, or
in the next couple days, whichever comes first.

Thanks,
jdl

On Thu, Jun 12, 2008 at 07:46:48PM -0500, Jon Loeliger wrote:

David Gibson wrote:

quoted

quoted

It'd be nice if we could keep the filename around for reporting here...

It would.  Well, I've been intending to clean up the input file
handling in several ways already, I'll see if that can be worked in.

With Scott's ACK on this, I intend commit David's patch.
So if you adjust the input file reporting here, please
do it as an "additional patch."

Oh, yes.  That was always my intention.

And for the record, I will also make an official tagged
release just after any such cleanup patch appears, or
in the next couple days, whichever comes first.

Ok.  I doubt I'll have that cleanup done in the next couple of days.
It can wait until after the tag.

-- 
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