[05/10] main: Add an option for path based targetting

Message ID 20181002060430.3344784-6-amitay@ozlabs.org
State Superseded
Headers show
Series
  • Device tree path base targeting
Related show

Commit Message

Amitay Isaacs Oct. 2, 2018, 6:04 a.m.
Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
---
 src/main.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

Comments

Alistair Popple Oct. 31, 2018, 4:01 a.m. | #1
Hi Amitay,

On Tuesday, 2 October 2018 4:04:26 PM AEDT Amitay Isaacs wrote:
> +static bool pathsel_add(char *format, ...) __attribute__((format (printf,
> 1, 2))); +
> +static bool pathsel_add(char *format, ...)
> +{
> +	va_list ap;
> +	char path[1024];
> +	int len;
> +
> +	va_start(ap, format);
> +
> +	len = vsnprintf(path, sizeof(path), format, ap);

I can't quite figure out what's going on here. As far as I can tell you're just 
passing a single string (optarg) to this function and adding it to the pathsel 
array. Why do we need the vsnprintf and the va_args? Am I missing something?

Thanks.

> +	if (len > sizeof(path)) {
> +		va_end(ap);
> +		return false;
> +	}
> +
> +	va_end(ap);
> +
> +	if (pathsel_count == MAX_PATH_ARGS) {
> +		fprintf(stderr, "Too many path arguments\n");
> +		return false;
> +	}
> +
> +	pathsel[pathsel_count] = strdup(path);
> +	assert(pathsel[pathsel_count]);
> +	pathsel_count++;
> +
> +	return true;
> +}
> +
>  static bool parse_options(int argc, char *argv[])
>  {
>  	int c;
> @@ -263,6 +300,7 @@ static bool parse_options(int argc, char *argv[])
>  		{"cpu",			required_argument,	NULL,	'l'},
>  #endif
>  		{"debug",		required_argument,	NULL,	'D'},
> +		{"path",		required_argument,	NULL,	'P'},
>  		{"shutup",		no_argument,		NULL,	'S'},
>  		{"version",		no_argument,		NULL,	'V'},
>  		{NULL,			0,			NULL,     0}
> @@ -275,7 +313,7 @@ static bool parse_options(int argc, char *argv[])
>  	memset(l_list, 0, sizeof(l_list));
> 
>  	do {
> -		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:SV" PPC_OPTS,
> +		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:P:SV" PPC_OPTS,
>  				long_opts, NULL);
>  		if (c == -1)
>  			break;
> @@ -361,6 +399,11 @@ static bool parse_options(int argc, char *argv[])
>  				fprintf(stderr, "Invalid slave address '%s'\n", optarg);
>  			break;
> 
> +		case 'P':
> +			if (!pathsel_add(optarg))
> +				opt_error = true;
> +			break;
> +
>  		case 'S':
>  			progress_shutup();
>  			break;
> @@ -647,6 +690,11 @@ static bool target_selection(void)
>  			target_unselect(fsi);
>  	}
> 
> +	if (pathsel_count) {
> +		if (!path_target_parse(pathsel, pathsel_count))
> +			return false;
> +	}
> +
>  	return true;
>  }
Amitay Isaacs Oct. 31, 2018, 5:14 a.m. | #2
On Wed, 2018-10-31 at 15:01 +1100, Alistair Popple wrote:
> Hi Amitay,
> 
> On Tuesday, 2 October 2018 4:04:26 PM AEDT Amitay Isaacs wrote:
> > +static bool pathsel_add(char *format, ...) __attribute__((format
> > (printf,
> > 1, 2))); +
> > +static bool pathsel_add(char *format, ...)
> > +{
> > +	va_list ap;
> > +	char path[1024];
> > +	int len;
> > +
> > +	va_start(ap, format);
> > +
> > +	len = vsnprintf(path, sizeof(path), format, ap);
> 
> I can't quite figure out what's going on here. As far as I can tell
> you're just 
> passing a single string (optarg) to this function and adding it to
> the pathsel 
> array. Why do we need the vsnprintf and the va_args? Am I missing
> something?

You have to look at the next patch in the series! :-)

For converting -p/-c/-t options I need to pass on specific path
patterns and that's where format strings are used.

> 
> Thanks.
> 
> > +	if (len > sizeof(path)) {
> > +		va_end(ap);
> > +		return false;
> > +	}
> > +
> > +	va_end(ap);
> > +
> > +	if (pathsel_count == MAX_PATH_ARGS) {
> > +		fprintf(stderr, "Too many path arguments\n");
> > +		return false;
> > +	}
> > +
> > +	pathsel[pathsel_count] = strdup(path);
> > +	assert(pathsel[pathsel_count]);
> > +	pathsel_count++;
> > +
> > +	return true;
> > +}
> > +
> >  static bool parse_options(int argc, char *argv[])
> >  {
> >  	int c;
> > @@ -263,6 +300,7 @@ static bool parse_options(int argc, char
> > *argv[])
> >  		{"cpu",			required_argument,	NUL
> > L,	'l'},
> >  #endif
> >  		{"debug",		required_argument,	NULL,	'D'
> > },
> > +		{"path",		required_argument,	NULL,	'P'
> > },
> >  		{"shutup",		no_argument,		NULL,	'S'
> > },
> >  		{"version",		no_argument,		NULL,	'V'
> > },
> >  		{NULL,			0,			NUL
> > L,     0}
> > @@ -275,7 +313,7 @@ static bool parse_options(int argc, char
> > *argv[])
> >  	memset(l_list, 0, sizeof(l_list));
> > 
> >  	do {
> > -		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:SV"
> > PPC_OPTS,
> > +		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:P:SV"
> > PPC_OPTS,
> >  				long_opts, NULL);
> >  		if (c == -1)
> >  			break;
> > @@ -361,6 +399,11 @@ static bool parse_options(int argc, char
> > *argv[])
> >  				fprintf(stderr, "Invalid slave address
> > '%s'\n", optarg);
> >  			break;
> > 
> > +		case 'P':
> > +			if (!pathsel_add(optarg))
> > +				opt_error = true;
> > +			break;
> > +
> >  		case 'S':
> >  			progress_shutup();
> >  			break;
> > @@ -647,6 +690,11 @@ static bool target_selection(void)
> >  			target_unselect(fsi);
> >  	}
> > 
> > +	if (pathsel_count) {
> > +		if (!path_target_parse(pathsel, pathsel_count))
> > +			return false;
> > +	}
> > +
> >  	return true;
> >  }
> 
> 

Amitay.
Alistair Popple Oct. 31, 2018, 5:49 a.m. | #3
On Wednesday, 31 October 2018 3:01:19 PM AEDT Alistair Popple wrote:
> Hi Amitay,
> 
> On Tuesday, 2 October 2018 4:04:26 PM AEDT Amitay Isaacs wrote:
> > +static bool pathsel_add(char *format, ...) __attribute__((format (printf,
> > 1, 2))); +
> > +static bool pathsel_add(char *format, ...)
> > +{
> > +	va_list ap;
> > +	char path[1024];
> > +	int len;
> > +
> > +	va_start(ap, format);
> > +
> > +	len = vsnprintf(path, sizeof(path), format, ap);
> 
> I can't quite figure out what's going on here. As far as I can tell you're
> just passing a single string (optarg) to this function and adding it to the
> pathsel array. Why do we need the vsnprintf and the va_args? Am I missing
> something?

Oh, I missed it's use in the subsequent patch. However what happens if the 
path on the command line contains a "%"? I get the below error:

$ ./pdbg -P "fsi0/pib%d" getscom 0xf000f
Invalid index '80'

So it seems like the vnsprintf is going to interpret the path as a format 
string which I'd like to see fixed. Thanks!

- Alistair
 
> Thanks.
> 
> > +	if (len > sizeof(path)) {
> > +		va_end(ap);
> > +		return false;
> > +	}
> > +
> > +	va_end(ap);
> > +
> > +	if (pathsel_count == MAX_PATH_ARGS) {
> > +		fprintf(stderr, "Too many path arguments\n");
> > +		return false;
> > +	}
> > +
> > +	pathsel[pathsel_count] = strdup(path);
> > +	assert(pathsel[pathsel_count]);
> > +	pathsel_count++;
> > +
> > +	return true;
> > +}
> > +
> > 
> >  static bool parse_options(int argc, char *argv[])
> >  {
> >  
> >  	int c;
> > 
> > @@ -263,6 +300,7 @@ static bool parse_options(int argc, char *argv[])
> > 
> >  		{"cpu",			required_argument,	NULL,	'l'},
> >  
> >  #endif
> >  
> >  		{"debug",		required_argument,	NULL,	'D'},
> > 
> > +		{"path",		required_argument,	NULL,	'P'},
> > 
> >  		{"shutup",		no_argument,		NULL,	'S'},
> >  		{"version",		no_argument,		NULL,	'V'},
> >  		{NULL,			0,			NULL,     0}
> > 
> > @@ -275,7 +313,7 @@ static bool parse_options(int argc, char *argv[])
> > 
> >  	memset(l_list, 0, sizeof(l_list));
> >  	
> >  	do {
> > 
> > -		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:SV" PPC_OPTS,
> > +		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:P:SV" PPC_OPTS,
> > 
> >  				long_opts, NULL);
> >  		
> >  		if (c == -1)
> >  		
> >  			break;
> > 
> > @@ -361,6 +399,11 @@ static bool parse_options(int argc, char *argv[])
> > 
> >  				fprintf(stderr, "Invalid slave address '%s'\n", optarg);
> >  			
> >  			break;
> > 
> > +		case 'P':
> > +			if (!pathsel_add(optarg))
> > +				opt_error = true;
> > +			break;
> > +
> > 
> >  		case 'S':
> >  			progress_shutup();
> >  			break;
> > 
> > @@ -647,6 +690,11 @@ static bool target_selection(void)
> > 
> >  			target_unselect(fsi);
> >  	
> >  	}
> > 
> > +	if (pathsel_count) {
> > +		if (!path_target_parse(pathsel, pathsel_count))
> > +			return false;
> > +	}
> > +
> > 
> >  	return true;
> >  
> >  }

Patch

diff --git a/src/main.c b/src/main.c
index f89ff6b..d57ce71 100644
--- a/src/main.c
+++ b/src/main.c
@@ -38,6 +38,7 @@ 
 #include "optcmd.h"
 #include "progress.h"
 #include "util.h"
+#include "path.h"
 
 #define PR_ERROR(x, args...) \
 	pdbg_log(PDBG_ERROR, x, ##args)
@@ -76,6 +77,11 @@  static int **processorsel[MAX_PROCESSORS];
 static int *chipsel[MAX_PROCESSORS][MAX_CHIPS];
 static int threadsel[MAX_PROCESSORS][MAX_CHIPS][MAX_THREADS];
 
+#define MAX_PATH_ARGS	16
+
+static const char *pathsel[MAX_PATH_ARGS];
+static int pathsel_count;
+
 static int probe(void);
 
 /* TODO: We are repeating ourselves here. A little bit more macro magic could
@@ -150,6 +156,7 @@  static void print_usage(char *pname)
 #ifdef TARGET_PPC
 	printf("\t-l, --cpu=<0-%d>|<range>|<list>\n", MAX_PROCESSORS-1);
 #endif
+	printf("\t-P, --path=<device tree node spec>\n");
 	printf("\t-a, --all\n");
 	printf("\t\tRun command on all possible processors/chips/threads (default)\n");
 	printf("\t-b, --backend=backend\n");
@@ -240,6 +247,36 @@  void pir_map(int pir, int *chip, int *core, int *thread) {}
 #define PPC_OPTS
 #endif
 
+static bool pathsel_add(char *format, ...) __attribute__((format (printf, 1, 2)));
+
+static bool pathsel_add(char *format, ...)
+{
+	va_list ap;
+	char path[1024];
+	int len;
+
+	va_start(ap, format);
+
+	len = vsnprintf(path, sizeof(path), format, ap);
+	if (len > sizeof(path)) {
+		va_end(ap);
+		return false;
+	}
+
+	va_end(ap);
+
+	if (pathsel_count == MAX_PATH_ARGS) {
+		fprintf(stderr, "Too many path arguments\n");
+		return false;
+	}
+
+	pathsel[pathsel_count] = strdup(path);
+	assert(pathsel[pathsel_count]);
+	pathsel_count++;
+
+	return true;
+}
+
 static bool parse_options(int argc, char *argv[])
 {
 	int c;
@@ -263,6 +300,7 @@  static bool parse_options(int argc, char *argv[])
 		{"cpu",			required_argument,	NULL,	'l'},
 #endif
 		{"debug",		required_argument,	NULL,	'D'},
+		{"path",		required_argument,	NULL,	'P'},
 		{"shutup",		no_argument,		NULL,	'S'},
 		{"version",		no_argument,		NULL,	'V'},
 		{NULL,			0,			NULL,     0}
@@ -275,7 +313,7 @@  static bool parse_options(int argc, char *argv[])
 	memset(l_list, 0, sizeof(l_list));
 
 	do {
-		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:SV" PPC_OPTS,
+		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:P:SV" PPC_OPTS,
 				long_opts, NULL);
 		if (c == -1)
 			break;
@@ -361,6 +399,11 @@  static bool parse_options(int argc, char *argv[])
 				fprintf(stderr, "Invalid slave address '%s'\n", optarg);
 			break;
 
+		case 'P':
+			if (!pathsel_add(optarg))
+				opt_error = true;
+			break;
+
 		case 'S':
 			progress_shutup();
 			break;
@@ -647,6 +690,11 @@  static bool target_selection(void)
 			target_unselect(fsi);
 	}
 
+	if (pathsel_count) {
+		if (!path_target_parse(pathsel, pathsel_count))
+			return false;
+	}
+
 	return true;
 }