[RESEND DTC PATCH 2/2] Add support for binary includes.

[RESEND DTC PATCH 2/2] Add support for binary includes.

[Scott Wood]

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

node {
	prop = /bin-include/ "path/to/data";
};

Search paths are not yet implemented; non-absolute lookups are relative to
the directory from which dtc was invoked.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
Apologies if you get this twice, but AFAICT the original got eaten by our
mail server.

 dtc-lexer.l  |    6 ++++++
 dtc-parser.y |   26 ++++++++++++++++++++++++++
 dtc.h        |    1 +
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/dtc-lexer.l b/dtc-lexer.l
index c811b22..1f3e6d6 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -190,6 +190,12 @@ static int dts_version; /* = 0 */
 			return DT_PROPNODENAME;
 		}
 
+"/bin-include/"	{
+			yylloc.filenum = srcpos_filenum;
+			yylloc.first_line = yylineno;
+			DPRINT("Binary Include\n");
+			return DT_BININCLUDE;
+		}
 
 <*>[[:space:]]+	/* eat whitespace */
 
diff --git a/dtc-parser.y b/dtc-parser.y
index 4a0181d..c7ed715 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -21,6 +21,8 @@
 %locations
 
 %{
+#include <sys/stat.h>
+
 #include "dtc.h"
 #include "srcpos.h"
 
@@ -58,6 +60,7 @@ extern struct boot_info *the_boot_info;
 %token <data> DT_STRING
 %token <labelref> DT_LABEL
 %token <labelref> DT_REF
+%token DT_BININCLUDE
 
 %type <data> propdata
 %type <data> propdataprefix
@@ -196,6 +199,29 @@ propdata:
 		{
 			$$ = data_add_marker($1, REF_PATH, $2);
 		}
+	| propdataprefix DT_BININCLUDE DT_STRING
+		{
+			struct stat st;
+			FILE *f;
+			int fd;
+			
+			f = fopen($3.val, "rb");
+			if (!f) {
+				yyerrorf("Cannot open file \"%s\": %s",
+				         $3.val, strerror(errno));
+				YYERROR;
+			}
+
+			fd = fileno(f);
+			if (fstat(fd, &st) < 0) {
+				yyerrorf("Cannot stat file \"%s\": %s",
+				         $3.val, strerror(errno));
+				YYERROR;
+			}
+
+			$$ = data_merge($1, data_copy_file(f, st.st_size));
+			fclose(f);
+		}
 	| propdata DT_LABEL
 		{
 			$$ = data_add_marker($1, LABEL, $2);
diff --git a/dtc.h b/dtc.h
index 9b89689..87b5bb1 100644
--- a/dtc.h
+++ b/dtc.h
@@ -138,6 +138,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_bin_include(const char *filename);
 
 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

Re: [RESEND DTC PATCH 2/2] Add support for binary includes.

[David Gibson]

On Thu, Dec 20, 2007 at 01:52:59PM -0600, Scott Wood wrote:
> A property's data can be populated with a file's contents
> as follows:
> 
> node {
> 	prop = /bin-include/ "path/to/data";
> };

I'd be inclined to use /incbin/ rather than /bin-include/.  It's only
slightly less obvious, but it's then the same as the gas pseudo-op as
well as being a little briefer.

> Search paths are not yet implemented; non-absolute lookups are relative to
> the directory from which dtc was invoked.

Hrm.  I think that's a bit too bogus.  Although it's rather more work
to implement, I think we have to make relative paths relative to the
location of the dts file until search paths are implemented.

> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> Apologies if you get this twice, but AFAICT the original got eaten by our
> mail server.
> 
>  dtc-lexer.l  |    6 ++++++
>  dtc-parser.y |   26 ++++++++++++++++++++++++++
>  dtc.h        |    1 +
>  3 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index c811b22..1f3e6d6 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -190,6 +190,12 @@ static int dts_version; /* = 0 */
>  			return DT_PROPNODENAME;
>  		}
>  
> +"/bin-include/"	{
> +			yylloc.filenum = srcpos_filenum;
> +			yylloc.first_line = yylineno;
> +			DPRINT("Binary Include\n");
> +			return DT_BININCLUDE;
> +		}
>  
>  <*>[[:space:]]+	/* eat whitespace */
>  
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 4a0181d..c7ed715 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -21,6 +21,8 @@
>  %locations
>  
>  %{
> +#include <sys/stat.h>
> +
>  #include "dtc.h"
>  #include "srcpos.h"
>  
> @@ -58,6 +60,7 @@ extern struct boot_info *the_boot_info;
>  %token <data> DT_STRING
>  %token <labelref> DT_LABEL
>  %token <labelref> DT_REF
> +%token DT_BININCLUDE
>  
>  %type <data> propdata
>  %type <data> propdataprefix
> @@ -196,6 +199,29 @@ propdata:
>  		{
>  			$$ = data_add_marker($1, REF_PATH, $2);
>  		}
> +	| propdataprefix DT_BININCLUDE DT_STRING
> +		{
> +			struct stat st;
> +			FILE *f;
> +			int fd;
> +			
> +			f = fopen($3.val, "rb");
> +			if (!f) {
> +				yyerrorf("Cannot open file \"%s\": %s",
> +				         $3.val, strerror(errno));
> +				YYERROR;

Hrm.  I'm not sure that being unable to open the file should cause a
*parse* error which is what YYERROR will do.  Probably better to print
an error message, but let the parsing continue, with the property
value being as though the file were empty.

> +			}
> +
> +			fd = fileno(f);
> +			if (fstat(fd, &st) < 0) {
> +				yyerrorf("Cannot stat file \"%s\": %s",
> +				         $3.val, strerror(errno));
> +				YYERROR;
> +			}

I'm also not sure that stat()ing the file is a good way to get the
size.  This requires that the included file be a regular file with a
sane st_size value, and I can imagine cases where it might be useful
to incbin from a /dev node or other special file.  Obviosuly
implementing that will require work to data_copy_file().

Actually, I think the way to go here would be to have two variants of
the incbin directive:  one which takes just a filename and includes
the whole file contents, another which takes a filename and a number
and includes just the first N bytes of the file.

> +			$$ = data_merge($1, data_copy_file(f, st.st_size));
> +			fclose(f);
> +		}
>  	| propdata DT_LABEL
>  		{
>  			$$ = data_add_marker($1, LABEL, $2);
> diff --git a/dtc.h b/dtc.h
> index 9b89689..87b5bb1 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -138,6 +138,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_bin_include(const char *filename);

This looks like a hangover from an earlier version.

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

Re: [RESEND DTC PATCH 2/2] Add support for binary includes.

[Scott Wood]

On Fri, Dec 21, 2007 at 11:29:22AM +1100, David Gibson wrote:
> On Thu, Dec 20, 2007 at 01:52:59PM -0600, Scott Wood wrote:
> > A property's data can be populated with a file's contents
> > as follows:
> > 
> > node {
> > 	prop = /bin-include/ "path/to/data";
> > };
> 
> I'd be inclined to use /incbin/ rather than /bin-include/.  It's only
> slightly less obvious, but it's then the same as the gas pseudo-op as
> well as being a little briefer.

OK.

> > Search paths are not yet implemented; non-absolute lookups are relative to
> > the directory from which dtc was invoked.
> 
> Hrm.  I think that's a bit too bogus.  Although it's rather more work
> to implement, I think we have to make relative paths relative to the
> location of the dts file until search paths are implemented.

OK.  I was being lazy. :-P

> > +	| propdataprefix DT_BININCLUDE DT_STRING
> > +		{
> > +			struct stat st;
> > +			FILE *f;
> > +			int fd;
> > +			
> > +			f = fopen($3.val, "rb");
> > +			if (!f) {
> > +				yyerrorf("Cannot open file \"%s\": %s",
> > +				         $3.val, strerror(errno));
> > +				YYERROR;
> 
> Hrm.  I'm not sure that being unable to open the file should cause a
> *parse* error which is what YYERROR will do.  Probably better to print
> an error message, but let the parsing continue, with the property
> value being as though the file were empty.

Yeah, I wanted something that would cause dtc to return an error code,
and it doesn't seem that calling yyerror(f) will do that at present.  I
guess I should fix that rather than overload YYERROR.

> 
> > +			}
> > +
> > +			fd = fileno(f);
> > +			if (fstat(fd, &st) < 0) {
> > +				yyerrorf("Cannot stat file \"%s\": %s",
> > +				         $3.val, strerror(errno));
> > +				YYERROR;
> > +			}
> 
> I'm also not sure that stat()ing the file is a good way to get the
> size.  This requires that the included file be a regular file with a
> sane st_size value, and I can imagine cases where it might be useful
> to incbin from a /dev node or other special file.  Obviosuly
> implementing that will require work to data_copy_file().

Hmm...  do you have a use case in mind?

> Actually, I think the way to go here would be to have two variants of
> the incbin directive:  one which takes just a filename and includes
> the whole file contents, another which takes a filename and a number
> and includes just the first N bytes of the file.

Maybe.  /incbinrange/ "path/name" start len?

> > diff --git a/dtc.h b/dtc.h
> > index 9b89689..87b5bb1 100644
> > --- a/dtc.h
> > +++ b/dtc.h
> > @@ -138,6 +138,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_bin_include(const char *filename);
> 
> This looks like a hangover from an earlier version.

Oops, yes.

-Scott

Re: [RESEND DTC PATCH 2/2] Add support for binary includes.

[David Gibson]

On Fri, Dec 21, 2007 at 11:09:21AM -0600, Scott Wood wrote:
> On Fri, Dec 21, 2007 at 11:29:22AM +1100, David Gibson wrote:
> > On Thu, Dec 20, 2007 at 01:52:59PM -0600, Scott Wood wrote:
> > > A property's data can be populated with a file's contents
> > > as follows:
> > > 
> > > node {
> > > 	prop = /bin-include/ "path/to/data";
> > > };
> > 
> > I'd be inclined to use /incbin/ rather than /bin-include/.  It's only
> > slightly less obvious, but it's then the same as the gas pseudo-op as
> > well as being a little briefer.
> 
> OK.
> 
> > > Search paths are not yet implemented; non-absolute lookups are relative to
> > > the directory from which dtc was invoked.
> > 
> > Hrm.  I think that's a bit too bogus.  Although it's rather more work
> > to implement, I think we have to make relative paths relative to the
> > location of the dts file until search paths are implemented.
> 
> OK.  I was being lazy. :-P

In general I'd approve, but having to invoke dtc in the right place
for the dts file is a bit too big a usability problem, I think.

> > > +	| propdataprefix DT_BININCLUDE DT_STRING
> > > +		{
> > > +			struct stat st;
> > > +			FILE *f;
> > > +			int fd;
> > > +			
> > > +			f = fopen($3.val, "rb");
> > > +			if (!f) {
> > > +				yyerrorf("Cannot open file \"%s\": %s",
> > > +				         $3.val, strerror(errno));
> > > +				YYERROR;
> > 
> > Hrm.  I'm not sure that being unable to open the file should cause a
> > *parse* error which is what YYERROR will do.  Probably better to print
> > an error message, but let the parsing continue, with the property
> > value being as though the file were empty.
> 
> Yeah, I wanted something that would cause dtc to return an error code,
> and it doesn't seem that calling yyerror(f) will do that at present.  I
> guess I should fix that rather than overload YYERROR.

No.  As per the yacc interface, yyerror() prints only, it doesn't
terminate.

You could use die(), although that might be an excessively scary
message for the situation.

> > > +			}
> > > +
> > > +			fd = fileno(f);
> > > +			if (fstat(fd, &st) < 0) {
> > > +				yyerrorf("Cannot stat file \"%s\": %s",
> > > +				         $3.val, strerror(errno));
> > > +				YYERROR;
> > > +			}
> > 
> > I'm also not sure that stat()ing the file is a good way to get the
> > size.  This requires that the included file be a regular file with a
> > sane st_size value, and I can imagine cases where it might be useful
> > to incbin from a /dev node or other special file.  Obviosuly
> > implementing that will require work to data_copy_file().
> 
> Hmm...  do you have a use case in mind?

Nothing really specific.  I'm thinking of a dts that maybe pulls in
some blobs from a pre-existing firmware, by sucking in files from
/proc/device-tree.  Or maybe something to produce a dts for a guest
under a hypervisor that takes an image of a real NVRAM or other device
to embed in the tree as a virtual NVRAM for the guest.

It's not a big deal, but since it shouldn't be that hard in principle
to read in a whole file without getting an st_size first.

> > Actually, I think the way to go here would be to have two variants of
> > the incbin directive:  one which takes just a filename and includes
> > the whole file contents, another which takes a filename and a number
> > and includes just the first N bytes of the file.
> 
> Maybe.  /incbinrange/ "path/name" start len?

I'd prefer to avoid two different keywords if possible.  I'll see if I
can think of a reasonable syntax.

> > > diff --git a/dtc.h b/dtc.h
> > > index 9b89689..87b5bb1 100644
> > > --- a/dtc.h
> > > +++ b/dtc.h
> > > @@ -138,6 +138,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_bin_include(const char *filename);
> > 
> > This looks like a hangover from an earlier version.
> 
> Oops, yes.
> 
> -Scott
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

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

Re: [RESEND DTC PATCH 2/2] Add support for binary includes.

[Scott Wood]

On Sat, Dec 22, 2007 at 01:51:30PM +1100, David Gibson wrote:
> On Fri, Dec 21, 2007 at 11:09:21AM -0600, Scott Wood wrote:
> > OK.  I was being lazy. :-P
> 
> In general I'd approve, but having to invoke dtc in the right place
> for the dts file is a bit too big a usability problem, I think.

Yeah, I agree.  It looks like the existing /include/ has the same
problem, BTW.

> > Yeah, I wanted something that would cause dtc to return an error code,
> > and it doesn't seem that calling yyerror(f) will do that at present.  I
> > guess I should fix that rather than overload YYERROR.
> 
> No.  As per the yacc interface, yyerror() prints only, it doesn't
> terminate.

I don't mean terminate early, just set a flag indicating there were
errors, so it returns an error code once parsing is done.

> > > I'm also not sure that stat()ing the file is a good way to get the
> > > size.  This requires that the included file be a regular file with a
> > > sane st_size value, and I can imagine cases where it might be useful
> > > to incbin from a /dev node or other special file.  Obviosuly
> > > implementing that will require work to data_copy_file().
> > 
> > Hmm...  do you have a use case in mind?
> 
> Nothing really specific.  I'm thinking of a dts that maybe pulls in
> some blobs from a pre-existing firmware, by sucking in files from
> /proc/device-tree.

'ls -l /proc/device-tree' seems to indicate that stat would work fine
there (and fstree.c uses it).

> Or maybe something to produce a dts for a guest under a hypervisor that
> takes an image of a real NVRAM or other device to embed in the tree as
> a virtual NVRAM for the guest.

OK.

> > > Actually, I think the way to go here would be to have two variants of
> > > the incbin directive:  one which takes just a filename and includes
> > > the whole file contents, another which takes a filename and a number
> > > and includes just the first N bytes of the file.
> > 
> > Maybe.  /incbinrange/ "path/name" start len?
> 
> I'd prefer to avoid two different keywords if possible.  I'll see if I
> can think of a reasonable syntax.

/incbin/("path")
/incbin/("path", start, len)
?

-Scott

Re: [RESEND DTC PATCH 2/2] Add support for binary includes.

[David Gibson]

On Sat, Dec 22, 2007 at 07:57:17AM -0600, Scott Wood wrote:
> On Sat, Dec 22, 2007 at 01:51:30PM +1100, David Gibson wrote:
> > On Fri, Dec 21, 2007 at 11:09:21AM -0600, Scott Wood wrote:
> > > OK.  I was being lazy. :-P
> > 
> > In general I'd approve, but having to invoke dtc in the right place
> > for the dts file is a bit too big a usability problem, I think.
> 
> Yeah, I agree.  It looks like the existing /include/ has the same
> problem, BTW.

Uh, yes, I guess it would.  I suppose we'd better fix that too.  We'll
see how bored I get at my parents' place over Christmas.

> > > Yeah, I wanted something that would cause dtc to return an error code,
> > > and it doesn't seem that calling yyerror(f) will do that at present.  I
> > > guess I should fix that rather than overload YYERROR.
> > 
> > No.  As per the yacc interface, yyerror() prints only, it doesn't
> > terminate.
> 
> I don't mean terminate early, just set a flag indicating there were
> errors, so it returns an error code once parsing is done.

Ok.  We don't have a really good mechanism for that at present - for
any of the input forms.  I was thinking about trying to hook that into
the checking framework - have a special sort of check that doesn't do
anything when invoked as a check, but can be "pre-failed" by the input
parsing code.

> > > > I'm also not sure that stat()ing the file is a good way to get the
> > > > size.  This requires that the included file be a regular file with a
> > > > sane st_size value, and I can imagine cases where it might be useful
> > > > to incbin from a /dev node or other special file.  Obviosuly
> > > > implementing that will require work to data_copy_file().
> > > 
> > > Hmm...  do you have a use case in mind?
> > 
> > Nothing really specific.  I'm thinking of a dts that maybe pulls in
> > some blobs from a pre-existing firmware, by sucking in files from
> > /proc/device-tree.
> 
> 'ls -l /proc/device-tree' seems to indicate that stat would work fine
> there (and fstree.c uses it).

Uh, yeah.  That's just me being an old-timer and remembering the days
when stat() didn't operate on /proc properly.

> > Or maybe something to produce a dts for a guest under a hypervisor that
> > takes an image of a real NVRAM or other device to embed in the tree as
> > a virtual NVRAM for the guest.
> 
> OK.
> 
> > > > Actually, I think the way to go here would be to have two variants of
> > > > the incbin directive:  one which takes just a filename and includes
> > > > the whole file contents, another which takes a filename and a number
> > > > and includes just the first N bytes of the file.
> > > 
> > > Maybe.  /incbinrange/ "path/name" start len?
> > 
> > I'd prefer to avoid two different keywords if possible.  I'll see if I
> > can think of a reasonable syntax.
> 
> /incbin/("path")
> /incbin/("path", start, len)

Hrm.  Not all that fond of those, but we'll see.  I guess
C-function-like syntax does make a certain amount of sense in the
context of the expression syntax we intend to introduce.

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