| Message ID | 20181002060430.3344784-6-amitay@ozlabs.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device tree path base targeting | expand |
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; > }
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.
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; > > > > }
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; }
Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- src/main.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-)