[PATCH 0/3] dtc: simplify command line invocation

[PATCH 0/3] dtc: simplify command line invocation

[Andre Przywara]

Hi,

most of time I find myself converting .dts to .dtb files and vice
versa, but having to explicitly specify -I and -O every time seems to
be unnecessary. The file names usually have proper extensions, also
a DTB file is easily recognized by its magic.
This series aims to make life easier, by trying to auto-detect the
file types if no explicit -I or -O parameters have been given.
The behaviour with explicit file types specified via these parameters
is not changed.
For details see the respective commit messages.

If that does not sound entirely stupid, I'd like to ask for these
patches to be merged.

Cheers,
Andre.

Andre Przywara (3):
  guess input file format based on file content or file name
  guess output file format
  Allow a second parameter to be the output file name

 dtc.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 72 insertions(+), 5 deletions(-)

-- 
1.8.4

[PATCH 1/3] guess input file format based on file content or file name

[Andre Przywara]

Always being required to specify the input file format can be quite
annoying, especially since a dtb is easily detected by its magic.
Also a dts can be guessed mostly by starting with a '/' character.
Looking at the file name extension also sounds useful as a hint.

Add heuristic file type guessing of the input file format in case
none has been specified on the command line.
The heuristics are as follows (in that order):
- Any issues with opening as a regular file lead to file name based
guessing: if the filename ends with .dts or .DTS, device tree source
text is assumed, .dtb or .DTB hint at a device tree blob.
- A directory will be treated as the /proc/device-tree type.
- If the first 4 bytes are the DTB magic, assume "dtb".
- If the first character is a '/', assume "dts".

For the majority of practical use cases this gets rid of the tedious
-I specification on the command line and simplifies actual typing of
dtc command lines.
Any explicit specification of the input type by using -I still avoids
any guessing, which resembles the current behaviour.

Signed-off-by: Andre Przywara <osp-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>
---
 dtc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/dtc.c b/dtc.c
index 8c4add6..cda563d 100644
--- a/dtc.c
+++ b/dtc.c
@@ -18,6 +18,8 @@
  *                                                                   USA
  */
 
+#include <sys/stat.h>
+
 #include "dtc.h"
 #include "srcpos.h"
 
@@ -104,10 +106,58 @@ static const char * const usage_opts_help[] = {
 	NULL,
 };
 
+static const char *guess_type_by_name(const char *fname, const char *fallback)
+{
+	const char *s;
+
+	s = strrchr(fname, '.');
+	if (s == NULL)
+		return fallback;
+	if (!strcmp(s, ".dts") || !strcmp(s, ".DTS"))
+		return "dts";
+	if (!strcmp(s, ".dtb") || !strcmp(s, ".DTB"))
+		return "dtb";
+	return fallback;
+}
+
+static const char *guess_input_format(const char *fname, const char *fallback)
+{
+	struct stat statbuf;
+	uint32_t magic;
+	FILE *f;
+
+	if (stat(fname, &statbuf))
+		return guess_type_by_name(fname, fallback);
+
+	if (S_ISDIR(statbuf.st_mode))
+		return "fs";
+
+	if (!S_ISREG(statbuf.st_mode))
+		return guess_type_by_name(fname, fallback);
+
+	f = fopen(fname, "r");
+	if (f == NULL)
+		return guess_type_by_name(fname, fallback);
+	if (fread(&magic, 4, 1, f) != 1) {
+		fclose(f);
+		return guess_type_by_name(fname, fallback);
+	}
+	fclose(f);
+
+	magic = fdt32_to_cpu(magic);
+	if (magic == FDT_MAGIC)
+		return "dtb";
+
+	if (magic >> 24 == '/')
+		return "dts";
+
+	return guess_type_by_name(fname, fallback);
+}
+
 int main(int argc, char *argv[])
 {
 	struct boot_info *bi;
-	const char *inform = "dts";
+	const char *inform = NULL;
 	const char *outform = "dts";
 	const char *outname = "-";
 	const char *depname = NULL;
@@ -213,6 +263,8 @@ int main(int argc, char *argv[])
 		fprintf(depfile, "%s:", outname);
 	}
 
+	if (inform == NULL)
+		inform = guess_input_format(arg, "dts");
 	if (streq(inform, "dts"))
 		bi = dt_from_source(arg);
 	else if (streq(inform, "fs"))
-- 
1.8.4

Re: [PATCH 1/3] guess input file format based on file content or file name

[David Gibson]

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

On Tue, May 26, 2015 at 12:11:21AM +0100, Andre Przywara wrote:
> Always being required to specify the input file format can be quite
> annoying, especially since a dtb is easily detected by its magic.
> Also a dts can be guessed mostly by starting with a '/' character.
> Looking at the file name extension also sounds useful as a hint.
> 
> Add heuristic file type guessing of the input file format in case
> none has been specified on the command line.
> The heuristics are as follows (in that order):
> - Any issues with opening as a regular file lead to file name based
> guessing: if the filename ends with .dts or .DTS, device tree source
> text is assumed, .dtb or .DTB hint at a device tree blob.
> - A directory will be treated as the /proc/device-tree type.
> - If the first 4 bytes are the DTB magic, assume "dtb".
> - If the first character is a '/', assume "dts".
> 
> For the majority of practical use cases this gets rid of the tedious
> -I specification on the command line and simplifies actual typing of
> dtc command lines.
> Any explicit specification of the input type by using -I still avoids
> any guessing, which resembles the current behaviour.
> 
> Signed-off-by: Andre Przywara <osp-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>

Sorry I've taken so long to reply to this.

Making decisions based on file extension makes be a little
uncomfortable so it's taken me a while to convince myself about this.
On balance, I think it's ok (since it can always be overridden).

However, there's a few details I'd like to see changed before
applying.


> ---
>  dtc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/dtc.c b/dtc.c
> index 8c4add6..cda563d 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -18,6 +18,8 @@
>   *                                                                   USA
>   */
>  
> +#include <sys/stat.h>
> +
>  #include "dtc.h"
>  #include "srcpos.h"
>  
> @@ -104,10 +106,58 @@ static const char * const usage_opts_help[] = {
>  	NULL,
>  };
>  
> +static const char *guess_type_by_name(const char *fname, const char *fallback)
> +{
> +	const char *s;
> +
> +	s = strrchr(fname, '.');
> +	if (s == NULL)
> +		return fallback;
> +	if (!strcmp(s, ".dts") || !strcmp(s, ".DTS"))
> +		return "dts";

Please use strcasecmp() instead of explicitly checking "dts" and
"DTS".  On a case-insensitive system, it's not implausible that you
could end up with ".Dts" or something.

> +	if (!strcmp(s, ".dtb") || !strcmp(s, ".DTB"))
> +		return "dtb";
> +	return fallback;
> +}
> +
> +static const char *guess_input_format(const char *fname, const char *fallback)
> +{
> +	struct stat statbuf;
> +	uint32_t magic;
> +	FILE *f;
> +
> +	if (stat(fname, &statbuf))
> +		return guess_type_by_name(fname, fallback);

First, please use an explicit != 0 here - it took me several reads
before I realised this was checking for a stat() failure.

I'd prefer not to guess if stat() fails - something is weird is going
on and I think it's better to force the user to specify explicitly in
this case.  The most likely case is that the user has specified the
wrong file name entirely here, so guessing by name isn't particularly
helpful.

> +	if (S_ISDIR(statbuf.st_mode))
> +		return "fs";
> +	if (!S_ISREG(statbuf.st_mode))
> +		return guess_type_by_name(fname, fallback);

Again, a non-regular file is a weird case, where I think we're better
off not guessing at all.

> +	f = fopen(fname, "r");
> +	if (f == NULL)
> +		return guess_type_by_name(fname, fallback);
> +	if (fread(&magic, 4, 1, f) != 1) {
> +		fclose(f);
> +		return guess_type_by_name(fname, fallback);
> +	}
> +	fclose(f);
> +
> +	magic = fdt32_to_cpu(magic);
> +	if (magic == FDT_MAGIC)
> +		return "dtb";
> +
> +	if (magic >> 24 == '/')
> +		return "dts";

This test is right in practice (because FDT is big-endian) but
misleading.  Since the relevant point is that the / is the very first
character - it's not a numerical value - check that directly rather
than looking at a possible byteswapped value.

Although.. this test is very weak anyway - a dts file could very
likely have whitespace or comments before the /dts-v1/ tag anyway.  I
don't think it's worth the bother.

> +
> +	return guess_type_by_name(fname, fallback);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	struct boot_info *bi;
> -	const char *inform = "dts";
> +	const char *inform = NULL;
>  	const char *outform = "dts";
>  	const char *outname = "-";
>  	const char *depname = NULL;
> @@ -213,6 +263,8 @@ int main(int argc, char *argv[])
>  		fprintf(depfile, "%s:", outname);
>  	}
>  
> +	if (inform == NULL)
> +		inform = guess_input_format(arg, "dts");
>  	if (streq(inform, "dts"))
>  		bi = dt_from_source(arg);
>  	else if (streq(inform, "fs"))

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] guess input file format based on file content or file name

[Andre Przywara]

On Tue, 30 Jun 2015 15:07:49 +1000
David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:

Hi David,

> On Tue, May 26, 2015 at 12:11:21AM +0100, Andre Przywara wrote:
> > Always being required to specify the input file format can be quite
> > annoying, especially since a dtb is easily detected by its magic.
> > Also a dts can be guessed mostly by starting with a '/' character.
> > Looking at the file name extension also sounds useful as a hint.
> > 
> > Add heuristic file type guessing of the input file format in case
> > none has been specified on the command line.
> > The heuristics are as follows (in that order):
> > - Any issues with opening as a regular file lead to file name based
> > guessing: if the filename ends with .dts or .DTS, device tree source
> > text is assumed, .dtb or .DTB hint at a device tree blob.
> > - A directory will be treated as the /proc/device-tree type.
> > - If the first 4 bytes are the DTB magic, assume "dtb".
> > - If the first character is a '/', assume "dts".
> > 
> > For the majority of practical use cases this gets rid of the tedious
> > -I specification on the command line and simplifies actual typing of
> > dtc command lines.
> > Any explicit specification of the input type by using -I still
> > avoids any guessing, which resembles the current behaviour.
> > 
> > Signed-off-by: Andre Przywara <osp-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>
> 
> Sorry I've taken so long to reply to this.

No worries, and thanks for taking a look!

> Making decisions based on file extension makes be a little
> uncomfortable so it's taken me a while to convince myself about this.
> On balance, I think it's ok (since it can always be overridden).
> 
> However, there's a few details I'd like to see changed before
> applying.

Thanks, those sound reasonable. I used that extra leap second to address
all your comments ;-)
In case of any error we will now drop back to the current default
behaviour, also I omitted the admittedly dodgy .dts guess based on the
first character being a '/'.
Will send out a v2 shortly.

Cheers,
Andre.


> 
> 
> > ---
> >  dtc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> > 
> > diff --git a/dtc.c b/dtc.c
> > index 8c4add6..cda563d 100644
> > --- a/dtc.c
> > +++ b/dtc.c
> > @@ -18,6 +18,8 @@
> >   *
> > USA */
> >  
> > +#include <sys/stat.h>
> > +
> >  #include "dtc.h"
> >  #include "srcpos.h"
> >  
> > @@ -104,10 +106,58 @@ static const char * const usage_opts_help[] =
> > { NULL,
> >  };
> >  
> > +static const char *guess_type_by_name(const char *fname, const
> > char *fallback) +{
> > +	const char *s;
> > +
> > +	s = strrchr(fname, '.');
> > +	if (s == NULL)
> > +		return fallback;
> > +	if (!strcmp(s, ".dts") || !strcmp(s, ".DTS"))
> > +		return "dts";
> 
> Please use strcasecmp() instead of explicitly checking "dts" and
> "DTS".  On a case-insensitive system, it's not implausible that you
> could end up with ".Dts" or something.
> 
> > +	if (!strcmp(s, ".dtb") || !strcmp(s, ".DTB"))
> > +		return "dtb";
> > +	return fallback;
> > +}
> > +
> > +static const char *guess_input_format(const char *fname, const
> > char *fallback) +{
> > +	struct stat statbuf;
> > +	uint32_t magic;
> > +	FILE *f;
> > +
> > +	if (stat(fname, &statbuf))
> > +		return guess_type_by_name(fname, fallback);
> 
> First, please use an explicit != 0 here - it took me several reads
> before I realised this was checking for a stat() failure.
> 
> I'd prefer not to guess if stat() fails - something is weird is going
> on and I think it's better to force the user to specify explicitly in
> this case.  The most likely case is that the user has specified the
> wrong file name entirely here, so guessing by name isn't particularly
> helpful.
> 
> > +	if (S_ISDIR(statbuf.st_mode))
> > +		return "fs";
> > +	if (!S_ISREG(statbuf.st_mode))
> > +		return guess_type_by_name(fname, fallback);
> 
> Again, a non-regular file is a weird case, where I think we're better
> off not guessing at all.
> 
> > +	f = fopen(fname, "r");
> > +	if (f == NULL)
> > +		return guess_type_by_name(fname, fallback);
> > +	if (fread(&magic, 4, 1, f) != 1) {
> > +		fclose(f);
> > +		return guess_type_by_name(fname, fallback);
> > +	}
> > +	fclose(f);
> > +
> > +	magic = fdt32_to_cpu(magic);
> > +	if (magic == FDT_MAGIC)
> > +		return "dtb";
> > +
> > +	if (magic >> 24 == '/')
> > +		return "dts";
> 
> This test is right in practice (because FDT is big-endian) but
> misleading.  Since the relevant point is that the / is the very first
> character - it's not a numerical value - check that directly rather
> than looking at a possible byteswapped value.
> 
> Although.. this test is very weak anyway - a dts file could very
> likely have whitespace or comments before the /dts-v1/ tag anyway.  I
> don't think it's worth the bother.
> 
> > +
> > +	return guess_type_by_name(fname, fallback);
> > +}
> > +
> >  int main(int argc, char *argv[])
> >  {
> >  	struct boot_info *bi;
> > -	const char *inform = "dts";
> > +	const char *inform = NULL;
> >  	const char *outform = "dts";
> >  	const char *outname = "-";
> >  	const char *depname = NULL;
> > @@ -213,6 +263,8 @@ int main(int argc, char *argv[])
> >  		fprintf(depfile, "%s:", outname);
> >  	}
> >  
> > +	if (inform == NULL)
> > +		inform = guess_input_format(arg, "dts");
> >  	if (streq(inform, "dts"))
> >  		bi = dt_from_source(arg);
> >  	else if (streq(inform, "fs"))
> 

[PATCH 2/3] guess output file format

[Andre Przywara]

If no output file type is specified via the -O parameter, guess the
desired file type by looking at the file name extension.
If that provides no useful hints, assume "dtb" as long as the input
type is "dts". Any other input type will lead to "dts" being used as
the guessed output type.
Any explicit specification of the output type will skip this guessing.

Signed-off-by: Andre Przywara <osp-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>
---
 dtc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/dtc.c b/dtc.c
index cda563d..ba34a06 100644
--- a/dtc.c
+++ b/dtc.c
@@ -158,7 +158,7 @@ int main(int argc, char *argv[])
 {
 	struct boot_info *bi;
 	const char *inform = NULL;
-	const char *outform = "dts";
+	const char *outform = NULL;
 	const char *outname = "-";
 	const char *depname = NULL;
 	bool force = false, sort = false;
@@ -265,6 +265,15 @@ int main(int argc, char *argv[])
 
 	if (inform == NULL)
 		inform = guess_input_format(arg, "dts");
+	if (outform == NULL) {
+		outform = guess_type_by_name(outname, NULL);
+		if (outform == NULL) {
+			if (streq(inform, "dts"))
+				outform = "dtb";
+			else
+				outform = "dts";
+		}
+	}
 	if (streq(inform, "dts"))
 		bi = dt_from_source(arg);
 	else if (streq(inform, "fs"))
-- 
1.8.4

[PATCH 3/3] allow a second parameter to be the output file name

[Andre Przywara]

If there is more than one file name specified on the command line,
assume it is the output file name.
This allows to use a more intuitive cp-style command line syntax,
which (together with the input type guessing) allows something like:
$ dtc input.dts output.dtb
(or the other way round) to cover the most common usage scenarios.

Signed-off-by: Andre Przywara <osp-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>
---
 dtc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/dtc.c b/dtc.c
index ba34a06..714d39e 100644
--- a/dtc.c
+++ b/dtc.c
@@ -159,7 +159,7 @@ int main(int argc, char *argv[])
 	struct boot_info *bi;
 	const char *inform = NULL;
 	const char *outform = NULL;
-	const char *outname = "-";
+	const char *outname = NULL;
 	const char *depname = NULL;
 	bool force = false, sort = false;
 	const char *arg;
@@ -244,9 +244,15 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	if (argc > (optind+1))
+	if (argc > (optind + 2))
 		usage("missing files");
-	else if (argc < (optind+1))
+
+	if (argc > (optind + 1) && outname == NULL)
+		outname = argv[optind + 1];
+	if (outname == NULL)
+		outname = "-";
+
+	if (argc < (optind + 1))
 		arg = "-";
 	else
 		arg = argv[optind];
-- 
1.8.4

Re: [PATCH 3/3] allow a second parameter to be the output file name

[David Gibson]

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

On Tue, May 26, 2015 at 12:11:23AM +0100, Andre Przywara wrote:
> If there is more than one file name specified on the command line,
> assume it is the output file name.
> This allows to use a more intuitive cp-style command line syntax,
> which (together with the input type guessing) allows something like:
> $ dtc input.dts output.dtb
> (or the other way round) to cover the most common usage scenarios.
> 
> Signed-off-by: Andre Przywara <osp-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>

Nack to this, sorry.

I'd prefer to model our command line on other compilers, and -o is a
more common convention there.

I would however, accept a patch to guess an output name based on input
name if -o is not specified, allowing the output name to be omitted
entirely in common cases.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] allow a second parameter to be the output file name

[Andre Przywara]

On Tue, 30 Jun 2015 15:11:19 +1000
David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:

Hi David,

> On Tue, May 26, 2015 at 12:11:23AM +0100, Andre Przywara wrote:
> > If there is more than one file name specified on the command line,
> > assume it is the output file name.
> > This allows to use a more intuitive cp-style command line syntax,
> > which (together with the input type guessing) allows something like:
> > $ dtc input.dts output.dtb
> > (or the other way round) to cover the most common usage scenarios.
> > 
> > Signed-off-by: Andre Przywara <osp-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>
> 
> Nack to this, sorry.
> 
> I'd prefer to model our command line on other compilers, and -o is a
> more common convention there.

Fair enough, I will drop this patch then.
Seems like: "dtc input.dts > output.dtb"  will do the trick, too and
it's just two characters more to type ;-)

> 
> I would however, accept a patch to guess an output name based on input
> name if -o is not specified, allowing the output name to be omitted
> entirely in common cases.

I'd rather keep the stdout behaviour in this case, I find writing to
unspecified filenames quite scary.

Cheers,
Andre.