[Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure

[Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure

[Yann E. MORIN]

Current, we only display the path that causes the paranoid failure. This
is sufficient, as we can fail only for -I and -L options, and it is thus
easy to infer from the path, which option is the culprit.

However, we're soon to add a new test for the -isystem option, and then
when a failure occurs, we would not know whether it was because of -I or
-isystem. Being able to differentiate both can be hugely useful to
track down the root cause for the unsafe path.

Make the check_unsafe_path() function accept a variable number of
arguments as a NULL-terminated list, to contain the offending options.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 toolchain/toolchain-wrapper.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
index 887058f..b8b3cbe 100644
--- a/toolchain/toolchain-wrapper.c
+++ b/toolchain/toolchain-wrapper.c
@@ -22,6 +22,7 @@
 #include <unistd.h>
 #include <stdlib.h>
 #include <errno.h>
+#include <stdarg.h>
 
 #ifdef BR_CCACHE
 static char ccache_path[PATH_MAX];
@@ -80,8 +81,10 @@ static char *predef_args[] = {
 #endif
 };
 
-static void check_unsafe_path(const char *path, int paranoid)
+static void check_unsafe_path(const char *path, int paranoid, ...)
 {
+	va_list ap;
+	int once;
 	char **c;
 	static char *unsafe_paths[] = {
 		"/lib", "/usr/include", "/usr/lib", "/usr/local/include", "/usr/local/lib", NULL,
@@ -92,6 +95,21 @@ static void check_unsafe_path(const char *path, int paranoid)
 			fprintf(stderr, "%s: %s: unsafe header/library path used in cross-compilation: '%s'\n",
 				program_invocation_short_name,
 				paranoid ? "ERROR" : "WARNING", path);
+			va_start(ap, paranoid);
+			once=1;
+			while(1) {
+				char *s = va_arg(ap, char*);
+				if(!s)
+					break;
+				if(once)
+					fprintf(stderr, "%s: options causing the issue:",
+						program_invocation_short_name);
+				once = 0;
+				fprintf(stderr, " '%s'", s);
+			}
+			if(!once)
+				fprintf(stderr, "\n");
+			va_end(ap);
 			if (paranoid)
 				exit(1);
 			continue;
@@ -237,9 +255,9 @@ int main(int argc, char **argv)
 			i++;
 			if (i == argc)
 				continue;
-			check_unsafe_path(argv[i], paranoid);
+			check_unsafe_path(argv[i], paranoid, argv[i-1], argv[i], NULL);
 		} else {
-			check_unsafe_path(argv[i] + 2, paranoid);
+			check_unsafe_path(argv[i] + 2, paranoid, argv[i], NULL);
 		}
 	}
 
-- 
2.7.4

[Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem

[Yann E. MORIN]

Some packages, like libbsd, use -isystem flags to provide so-called
overrides to the system include files. In this particular case, this
is used in a .pc file, then used by antoher package; pkgconf does not
mangle this path; and eventually that other package ends up using
/usr/include/bsd to search for headers.

Our current toolchain wrapper is limited to looking fo -I and -L, so
the paranoid check does not kick in.

Extend the paranoid check to also look for the -isystem option.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 toolchain/toolchain-wrapper.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
index b8b3cbe..8a9c3b3 100644
--- a/toolchain/toolchain-wrapper.c
+++ b/toolchain/toolchain-wrapper.c
@@ -241,17 +241,19 @@ int main(int argc, char **argv)
 	/* Check for unsafe library and header paths */
 	for (i = 1; i < argc; i++) {
 
-		/* Skip options that do not start with -I and -L */
-		if (strncmp(argv[i], "-I", 2) && strncmp(argv[i], "-L", 2))
+		/* Skip options that do not start with -I, -isystem or -L */
+		if (   strncmp(argv[i], "-I", 2)
+		    && strncmp(argv[i], "-L", 2)
+		    && strcmp(argv[i], "-isystem"))
 			continue;
 
-		/* We handle two cases: first the case where -I/-L and
-		 * the path are separated by one space and therefore
-		 * visible as two separate options, and then the case
-		 * where they are stuck together forming one single
-		 * option.
+		/* We handle two cases: first the case where -I/-L/-isystem
+		 * and the path are separated by one space and therefore
+		 * visible as two separate options, and then the case  where
+		 * they are stuck together forming one single option.
+		 * -isystem is necessarily in the first case.
 		 */
-		if (argv[i][2] == '\0') {
+		if (argv[i][2] == '\0' || argv[i][1] == 'i') {
 			i++;
 			if (i == argc)
 				continue;
-- 
2.7.4

[Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure

[Arnout Vandecappelle]

On 17-08-16 16:42, Yann E. MORIN wrote:
> Current, we only display the path that causes the paranoid failure. This
> is sufficient, as we can fail only for -I and -L options, and it is thus
> easy to infer from the path, which option is the culprit.
> 
> However, we're soon to add a new test for the -isystem option, and then
> when a failure occurs, we would not know whether it was because of -I or
> -isystem. Being able to differentiate both can be hugely useful to
> track down the root cause for the unsafe path.
> 
> Make the check_unsafe_path() function accept a variable number of
> arguments as a NULL-terminated list, to contain the offending options.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> ---
>  toolchain/toolchain-wrapper.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
> index 887058f..b8b3cbe 100644
> --- a/toolchain/toolchain-wrapper.c
> +++ b/toolchain/toolchain-wrapper.c
> @@ -22,6 +22,7 @@
>  #include <unistd.h>
>  #include <stdlib.h>
>  #include <errno.h>
> +#include <stdarg.h>
>  
>  #ifdef BR_CCACHE
>  static char ccache_path[PATH_MAX];
> @@ -80,8 +81,10 @@ static char *predef_args[] = {
>  #endif
>  };
>  
> -static void check_unsafe_path(const char *path, int paranoid)
> +static void check_unsafe_path(const char *path, int paranoid, ...)

 I'm not very happy with using varargs for such a simple case. How about:

static void check_unsafe_path(const char *arg, const char *path,
                              int paranoid, bool arg_includes_path)
...

>  {
> +	va_list ap;
> +	int once;
>  	char **c;
>  	static char *unsafe_paths[] = {
>  		"/lib", "/usr/include", "/usr/lib", "/usr/local/include", "/usr/local/lib", NULL,
> @@ -92,6 +95,21 @@ static void check_unsafe_path(const char *path, int paranoid)
>  			fprintf(stderr, "%s: %s: unsafe header/library path used in cross-compilation: '%s'\n",
>  				program_invocation_short_name,
>  				paranoid ? "ERROR" : "WARNING", path);

and here:

 			fprintf(stderr, "%s: %s: unsafe header/library path used in
cross-compilation: '%s%s%s'\n",
 				program_invocation_short_name,
 				paranoid ? "ERROR" : "WARNING",
				arg,
				arg_includes_path ? "" : " ",
				arg_includes_path ? "" : path);

> +			va_start(ap, paranoid);
> +			once=1;
> +			while(1) {
> +				char *s = va_arg(ap, char*);
> +				if(!s)
> +					break;
> +				if(once)
> +					fprintf(stderr, "%s: options causing the issue:",
> +						program_invocation_short_name);
> +				once = 0;
> +				fprintf(stderr, " '%s'", s);
> +			}
> +			if(!once)
> +				fprintf(stderr, "\n");
> +			va_end(ap);
>  			if (paranoid)
>  				exit(1);
>  			continue;
> @@ -237,9 +255,9 @@ int main(int argc, char **argv)
>  			i++;
>  			if (i == argc)
>  				continue;
> -			check_unsafe_path(argv[i], paranoid);
> +			check_unsafe_path(argv[i], paranoid, argv[i-1], argv[i], NULL);

			check_unsafe_path(argv[i-1], argv[i], paranoid, false);

>  		} else {
> -			check_unsafe_path(argv[i] + 2, paranoid);
> +			check_unsafe_path(argv[i] + 2, paranoid, argv[i], NULL);

			check_unsafe_path(argv[i], argv[i] + 2, paranoid, true);


 Regards,
 Arnout


>  		}
>  	}
>  
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem

[Arnout Vandecappelle]

On 17-08-16 16:42, Yann E. MORIN wrote:
> Some packages, like libbsd, use -isystem flags to provide so-called
> overrides to the system include files. In this particular case, this
> is used in a .pc file, then used by antoher package; pkgconf does not
> mangle this path; and eventually that other package ends up using
> /usr/include/bsd to search for headers.
> 
> Our current toolchain wrapper is limited to looking fo -I and -L, so
> the paranoid check does not kick in.
> 
> Extend the paranoid check to also look for the -isystem option.

 While we're at it: -idirafter, -iprefix, -iwithprefix, -iwithprefixbefore,
-isysroot, -imultilib, -iquote.

 And then there is -B, but if someone passes that, it's really broken :-) And
--sysroot, also interesting if that is passed. But I guess these things are
going a bit too far.


> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> ---
>  toolchain/toolchain-wrapper.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
> index b8b3cbe..8a9c3b3 100644
> --- a/toolchain/toolchain-wrapper.c
> +++ b/toolchain/toolchain-wrapper.c
> @@ -241,17 +241,19 @@ int main(int argc, char **argv)
>  	/* Check for unsafe library and header paths */
>  	for (i = 1; i < argc; i++) {
>  
> -		/* Skip options that do not start with -I and -L */
> -		if (strncmp(argv[i], "-I", 2) && strncmp(argv[i], "-L", 2))
> +		/* Skip options that do not start with -I, -isystem or -L */
> +		if (   strncmp(argv[i], "-I", 2)
> +		    && strncmp(argv[i], "-L", 2)
> +		    && strcmp(argv[i], "-isystem"))
>  			continue;
>  
> -		/* We handle two cases: first the case where -I/-L and
> -		 * the path are separated by one space and therefore
> -		 * visible as two separate options, and then the case
> -		 * where they are stuck together forming one single
> -		 * option.
> +		/* We handle two cases: first the case where -I/-L/-isystem
> +		 * and the path are separated by one space and therefore
> +		 * visible as two separate options, and then the case  where
> +		 * they are stuck together forming one single option.
> +		 * -isystem is necessarily in the first case.

 Unfortunately, that's not true. You can pass something like -isystemfoo and it
will add ./foo to the search path. Ain't gcc fun... I'll leave it as an exercise
to the reader to handle that case :-P

 Regards,
 Arnout

>  		 */
> -		if (argv[i][2] == '\0') {
> +		if (argv[i][2] == '\0' || argv[i][1] == 'i') {
>  			i++;
>  			if (i == argc)
>  				continue;
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem

[Yann E. MORIN]

Arnout, All,

On 2016-08-24 03:18 +0200, Arnout Vandecappelle spake thusly:
> On 17-08-16 16:42, Yann E. MORIN wrote:
> > Some packages, like libbsd, use -isystem flags to provide so-called
> > overrides to the system include files. In this particular case, this
> > is used in a .pc file, then used by antoher package; pkgconf does not
> > mangle this path; and eventually that other package ends up using
> > /usr/include/bsd to search for headers.
> > 
> > Our current toolchain wrapper is limited to looking fo -I and -L, so
> > the paranoid check does not kick in.
> > 
> > Extend the paranoid check to also look for the -isystem option.
> 
>  While we're at it: -idirafter, -iprefix, -iwithprefix, -iwithprefixbefore,
> -isysroot, -imultilib, -iquote.

Did you meant we should handle all of them now? Are were you listing
them for the future, when we encoutner issues with any if them?

>  And then there is -B, but if someone passes that, it's really broken :-) And
> --sysroot, also interesting if that is passed. But I guess these things are
> going a bit too far.

--sysroot is even more fun, as it can be written: --sysroot=dir , so
we'd need to take care of this as well...

Lotta fun in sight! ;-)

> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Arnout Vandecappelle <arnout@mind.be>
> > ---
> >  toolchain/toolchain-wrapper.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
> > index b8b3cbe..8a9c3b3 100644
> > --- a/toolchain/toolchain-wrapper.c
> > +++ b/toolchain/toolchain-wrapper.c
> > @@ -241,17 +241,19 @@ int main(int argc, char **argv)
> >  	/* Check for unsafe library and header paths */
> >  	for (i = 1; i < argc; i++) {
> >  
> > -		/* Skip options that do not start with -I and -L */
> > -		if (strncmp(argv[i], "-I", 2) && strncmp(argv[i], "-L", 2))
> > +		/* Skip options that do not start with -I, -isystem or -L */
> > +		if (   strncmp(argv[i], "-I", 2)
> > +		    && strncmp(argv[i], "-L", 2)
> > +		    && strcmp(argv[i], "-isystem"))
> >  			continue;
> >  
> > -		/* We handle two cases: first the case where -I/-L and
> > -		 * the path are separated by one space and therefore
> > -		 * visible as two separate options, and then the case
> > -		 * where they are stuck together forming one single
> > -		 * option.
> > +		/* We handle two cases: first the case where -I/-L/-isystem
> > +		 * and the path are separated by one space and therefore
> > +		 * visible as two separate options, and then the case  where
> > +		 * they are stuck together forming one single option.
> > +		 * -isystem is necessarily in the first case.
> 
>  Unfortunately, that's not true. You can pass something like -isystemfoo and it
> will add ./foo to the search path. Ain't gcc fun... I'll leave it as an exercise
> to the reader to handle that case :-P

I think we should not care too much about this, should we? In the end,
this would not be an unsafe path...

But I think my next iteration should cover all your comments (as well as
on the previous patch).

Thanks! ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem

[Arnout Vandecappelle]

On 24-08-16 16:12, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2016-08-24 03:18 +0200, Arnout Vandecappelle spake thusly:
>> On 17-08-16 16:42, Yann E. MORIN wrote:
>>> Some packages, like libbsd, use -isystem flags to provide so-called
>>> overrides to the system include files. In this particular case, this
>>> is used in a .pc file, then used by antoher package; pkgconf does not
>>> mangle this path; and eventually that other package ends up using
>>> /usr/include/bsd to search for headers.
>>>
>>> Our current toolchain wrapper is limited to looking fo -I and -L, so
>>> the paranoid check does not kick in.
>>>
>>> Extend the paranoid check to also look for the -isystem option.
>>
>>  While we're at it: -idirafter, -iprefix, -iwithprefix, -iwithprefixbefore,
>> -isysroot, -imultilib, -iquote.
> 
> Did you meant we should handle all of them now? Are were you listing
> them for the future, when we encoutner issues with any if them?

 I think it doesn't hurt to include these now. Though the prefix ones are a bit
iffy (you could pass -iprefix /usr/ -iwithprefix lib and this wouldn't be
captured by the paranoid check). But certainly -idirafter and -iquote should be
handled now IMHO.

> 
>>  And then there is -B, but if someone passes that, it's really broken :-) And
>> --sysroot, also interesting if that is passed. But I guess these things are
>> going a bit too far.
> 
> --sysroot is even more fun, as it can be written: --sysroot=dir , so
> we'd need to take care of this as well...

 Well, anything that is passing --sysroot is doing really crazy shit so chances
are our paranoid check is wrong anyway, so let's ignore that one.

> 
> Lotta fun in sight! ;-)
> 
>>> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
>>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>>> Cc: Arnout Vandecappelle <arnout@mind.be>
>>> ---
>>>  toolchain/toolchain-wrapper.c | 18 ++++++++++--------
>>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
>>> index b8b3cbe..8a9c3b3 100644
>>> --- a/toolchain/toolchain-wrapper.c
>>> +++ b/toolchain/toolchain-wrapper.c
>>> @@ -241,17 +241,19 @@ int main(int argc, char **argv)
>>>  	/* Check for unsafe library and header paths */
>>>  	for (i = 1; i < argc; i++) {
>>>  
>>> -		/* Skip options that do not start with -I and -L */
>>> -		if (strncmp(argv[i], "-I", 2) && strncmp(argv[i], "-L", 2))
>>> +		/* Skip options that do not start with -I, -isystem or -L */
>>> +		if (   strncmp(argv[i], "-I", 2)
>>> +		    && strncmp(argv[i], "-L", 2)
>>> +		    && strcmp(argv[i], "-isystem"))
>>>  			continue;
>>>  
>>> -		/* We handle two cases: first the case where -I/-L and
>>> -		 * the path are separated by one space and therefore
>>> -		 * visible as two separate options, and then the case
>>> -		 * where they are stuck together forming one single
>>> -		 * option.
>>> +		/* We handle two cases: first the case where -I/-L/-isystem
>>> +		 * and the path are separated by one space and therefore
>>> +		 * visible as two separate options, and then the case  where
>>> +		 * they are stuck together forming one single option.
>>> +		 * -isystem is necessarily in the first case.
>>
>>  Unfortunately, that's not true. You can pass something like -isystemfoo and it
>> will add ./foo to the search path. Ain't gcc fun... I'll leave it as an exercise
>> to the reader to handle that case :-P
> 
> I think we should not care too much about this, should we? In the end,
> this would not be an unsafe path...

 I gave -isystemfoo as an example because it looks so funny. but
-isystem/usr/lib is possible as well.

 Fortunately, your v2 handles that!

 Regards,
 Arnout

> 
> But I think my next iteration should cover all your comments (as well as
> on the previous patch).
> 
> Thanks! ;-)
> 
> Regards,
> Yann E. MORIN.
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure

[Yann E. MORIN]

Thomas, All,

On 2016-08-24 16:36 +0200, Thomas Petazzoni spake thusly:
> On Wed, 24 Aug 2016 16:19:29 +0200, Yann E. MORIN wrote:
> > Current, we only display the path that causes the paranoid failure. This
> > is sufficient, as we can fail only for -I and -L options, and it is thus
> > easy to infer from the path, which option is the culprit.
> > 
> > However, we're soon to add a new test for the -isystem option, and then
> > when a failure occurs, we would not know whether it was because of -I or
> > -isystem. Being able to differentiate both can be hugely useful to
> > track down the root cause for the unsafe path.
> > 
> > Add two new arguments to the check_unsafe_path() function: one with the
> > current-or-previous argument, one to specify whether it has the path in
> > it or not. Print that in the error message, instead of just the path.
> > 
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Arnout Vandecappelle <arnout@mind.be>
> 
> Seems like a good feature addition.
> 
> > -static void check_unsafe_path(const char *path, int paranoid)
> > +static void check_unsafe_path(const char *arg,
> > +			      const char *path,
> > +			      int paranoid,
> > +			      int arg_has_path)
> >  {
> > +	va_list ap;
> > +	int once;
> 
> Those variables are not needed I believe.

Dang. Stray variables from v1... Removed now!

> >  	char **c;
> >  	static char *unsafe_paths[] = {
> >  		"/lib", "/usr/include", "/usr/lib", "/usr/local/include", "/usr/local/lib", NULL,
> > @@ -89,9 +94,15 @@ static void check_unsafe_path(const char *path, int paranoid)
> >  
> >  	for (c = unsafe_paths; *c != NULL; c++) {
> >  		if (!strncmp(path, *c, strlen(*c))) {
> > -			fprintf(stderr, "%s: %s: unsafe header/library path used in cross-compilation: '%s'\n",
> > +			fprintf(stderr,
> > +				"%s: %s: "
> > +				"unsafe header/library path used in cross-compilation:"
> > +				" '%s%s%s'\n",
> 
> I'm not a big fan of splitting the format string.

Me neither, but the line was really overly long...

(And that's why I don;t like TABs and settign them to 8; 4 spaces ought
to be enough for everyone...)

> What about inverting
> the if() test in order to reduce the indentation level of the error
> case?

Done.

> 	for (c = unsafe_paths; *c != NULL; c++) {
> 		if (strncmp(path, *c, strlen(*c)))
> 			continue;
> 		fprintf(stderr,
> 			"%s: %s: unsafe header/library path used in cross-compilation: '%s%s%s'\n",
> 			....
> 	}
> 			
> >  				program_invocation_short_name,
> > -				paranoid ? "ERROR" : "WARNING", path);
> > +				paranoid ? "ERROR" : "WARNING",
> > +				arg,
> > +				arg_has_path ? "" : "' '",
> > +				arg_has_path ? "" : path);
> 
> I find this arg_has_path thing a bit tricky: in some cases "arg" will
> be just the argument, in some cases it is followed by the path. But I
> couldn't find a simple and nice alternate solution, so it's probably
> good as-is.

I'll add a bit of documentation to the function, so that it's more
obvious what it is doing. Still tricky, but better explained! ;-)

I've also already added a comment about the "' '" trick, too.

Regards,
Yann E. MORIN.

> Thanks!
> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure

[Thomas Petazzoni]

Hello,

On Wed, 24 Aug 2016 16:19:29 +0200, Yann E. MORIN wrote:
> Current, we only display the path that causes the paranoid failure. This
> is sufficient, as we can fail only for -I and -L options, and it is thus
> easy to infer from the path, which option is the culprit.
> 
> However, we're soon to add a new test for the -isystem option, and then
> when a failure occurs, we would not know whether it was because of -I or
> -isystem. Being able to differentiate both can be hugely useful to
> track down the root cause for the unsafe path.
> 
> Add two new arguments to the check_unsafe_path() function: one with the
> current-or-previous argument, one to specify whether it has the path in
> it or not. Print that in the error message, instead of just the path.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>

Seems like a good feature addition.

> -static void check_unsafe_path(const char *path, int paranoid)
> +static void check_unsafe_path(const char *arg,
> +			      const char *path,
> +			      int paranoid,
> +			      int arg_has_path)
>  {
> +	va_list ap;
> +	int once;

Those variables are not needed I believe.

>  	char **c;
>  	static char *unsafe_paths[] = {
>  		"/lib", "/usr/include", "/usr/lib", "/usr/local/include", "/usr/local/lib", NULL,
> @@ -89,9 +94,15 @@ static void check_unsafe_path(const char *path, int paranoid)
>  
>  	for (c = unsafe_paths; *c != NULL; c++) {
>  		if (!strncmp(path, *c, strlen(*c))) {
> -			fprintf(stderr, "%s: %s: unsafe header/library path used in cross-compilation: '%s'\n",
> +			fprintf(stderr,
> +				"%s: %s: "
> +				"unsafe header/library path used in cross-compilation:"
> +				" '%s%s%s'\n",

I'm not a big fan of splitting the format string. What about inverting
the if() test in order to reduce the indentation level of the error
case?

	for (c = unsafe_paths; *c != NULL; c++) {
		if (strncmp(path, *c, strlen(*c)))
			continue;
		fprintf(stderr,
			"%s: %s: unsafe header/library path used in cross-compilation: '%s%s%s'\n",
			....
	}
			
>  				program_invocation_short_name,
> -				paranoid ? "ERROR" : "WARNING", path);
> +				paranoid ? "ERROR" : "WARNING",
> +				arg,
> +				arg_has_path ? "" : "' '",
> +				arg_has_path ? "" : path);

I find this arg_has_path thing a bit tricky: in some cases "arg" will
be just the argument, in some cases it is followed by the path. But I
couldn't find a simple and nice alternate solution, so it's probably
good as-is.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure

[Yann E. MORIN]

Current, we only display the path that causes the paranoid failure. This
is sufficient, as we can fail only for -I and -L options, and it is thus
easy to infer from the path, which option is the culprit.

However, we're soon to add a new test for the -isystem option, and then
when a failure occurs, we would not know whether it was because of -I or
-isystem. Being able to differentiate both can be hugely useful to
track down the root cause for the unsafe path.

Add two new arguments to the check_unsafe_path() function: one with the
current-or-previous argument, one to specify whether it has the path in
it or not. Print that in the error message, instead of just the path.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>

---
Changes v1 -> v2;
  - don't use a variadic function; use explicit argumetns  (Arnout)
  - print it on a single line  (Arnout)
---
 toolchain/toolchain-wrapper.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
index 887058f..edade43 100644
--- a/toolchain/toolchain-wrapper.c
+++ b/toolchain/toolchain-wrapper.c
@@ -80,8 +80,13 @@ static char *predef_args[] = {
 #endif
 };
 
-static void check_unsafe_path(const char *path, int paranoid)
+static void check_unsafe_path(const char *arg,
+			      const char *path,
+			      int paranoid,
+			      int arg_has_path)
 {
+	va_list ap;
+	int once;
 	char **c;
 	static char *unsafe_paths[] = {
 		"/lib", "/usr/include", "/usr/lib", "/usr/local/include", "/usr/local/lib", NULL,
@@ -89,9 +94,15 @@ static void check_unsafe_path(const char *path, int paranoid)
 
 	for (c = unsafe_paths; *c != NULL; c++) {
 		if (!strncmp(path, *c, strlen(*c))) {
-			fprintf(stderr, "%s: %s: unsafe header/library path used in cross-compilation: '%s'\n",
+			fprintf(stderr,
+				"%s: %s: "
+				"unsafe header/library path used in cross-compilation:"
+				" '%s%s%s'\n",
 				program_invocation_short_name,
-				paranoid ? "ERROR" : "WARNING", path);
+				paranoid ? "ERROR" : "WARNING",
+				arg,
+				arg_has_path ? "" : "' '",
+				arg_has_path ? "" : path);
 			if (paranoid)
 				exit(1);
 			continue;
@@ -237,9 +248,9 @@ int main(int argc, char **argv)
 			i++;
 			if (i == argc)
 				continue;
-			check_unsafe_path(argv[i], paranoid);
+			check_unsafe_path(argv[i-1], argv[i], paranoid, 0);
 		} else {
-			check_unsafe_path(argv[i] + 2, paranoid);
+			check_unsafe_path(argv[i], argv[i] + 2, paranoid, 1);
 		}
 	}
 
-- 
2.7.4