| Message ID | 20170817142942.4328-1-christian.storm@siemens.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
Hi Christian, On 17/08/2017 16:29, Christian Storm wrote: > (1) disallow options' values starting with '-' except for > downloader, webserver, and suricatta doing their own > cmdline parsing. Otherwise, e.g., this command > $ swupdate -l -c -i <file> > installs <file> instead of checking it due to -l's > option value missing. > (2) abort on superfluous non-option cmdline arguments > as SWUpdate doesn't use them, probably an usage error. > (3) check sensible combinations with suricatta mode > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > core/swupdate.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/core/swupdate.c b/core/swupdate.c > index b01aadd..f364bce 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -599,6 +599,12 @@ int main(int argc, char **argv) > /* Process options with getopt */ > while ((c = getopt_long(argc, argv, main_options, > long_options, NULL)) != EOF) { > + if (optarg && *optarg == '-' && (c != 'd' && c != 'u' && c != 'w')) { > + /* An option's value starting with '-' is not allowed except I have largely used kernel multi-line comments in SWUpdate. Of course, this is not an issue, but before we are going into a jungle of different code-styles, we should stick to one of them. And then to: /* * Multi line * comments */ > + * for downloader, webserver, and suricatta doing their own > + * argv parsing. */ > + c = '?'; > + } > switch (c) { > case 'v': > loglevel = TRACELEVEL; > @@ -680,6 +686,12 @@ int main(int argc, char **argv) > } > } > > + if (optind < argc) { > + /* SWUpdate has no non-option arguments, fail on them */ > + usage(argv[0]); > + exit(1); > + } > + > /* > * Parameters are parsed: now performs plausibility > * tests before starting processes and threads > @@ -698,6 +710,18 @@ int main(int argc, char **argv) > exit(1); > } > > +#ifdef CONFIG_SURICATTA > + if (opt_u && (opt_c || opt_i > +#ifdef CONFIG_DOWNLOAD > + || opt_d > +#endif > + )) { > + fprintf(stderr, "invalid mode combination with suricatta.\n"); > + usage(argv[0]); > + exit(1); > + } > +#endif I am not convinced this is required. First, it can be that one starts SWUpdate in the same way, but in one case just adding "-c -i <file>". And then starts SWUpdate with all options. Of course, SWUpdate exits, but it does what it is supposed to do. Without this test, everything works as expected: SWUpdate checks for a sw-description inside the file provided with -i, parses and reports the result before exiting. I can also think that -d and -u are not mutually exclusive, but just a second server is available in case of Hawkbit failure (or viceversa). Best regards, Stefano Babic
Hi Stefano, > > (1) disallow options' values starting with '-' except for > > downloader, webserver, and suricatta doing their own > > cmdline parsing. Otherwise, e.g., this command > > $ swupdate -l -c -i <file> > > installs <file> instead of checking it due to -l's > > option value missing. > > (2) abort on superfluous non-option cmdline arguments > > as SWUpdate doesn't use them, probably an usage error. > > (3) check sensible combinations with suricatta mode > > > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > > --- > > core/swupdate.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/core/swupdate.c b/core/swupdate.c > > index b01aadd..f364bce 100644 > > --- a/core/swupdate.c > > +++ b/core/swupdate.c > > @@ -599,6 +599,12 @@ int main(int argc, char **argv) > > /* Process options with getopt */ > > while ((c = getopt_long(argc, argv, main_options, > > long_options, NULL)) != EOF) { > > + if (optarg && *optarg == '-' && (c != 'd' && c != 'u' && c != 'w')) { > > + /* An option's value starting with '-' is not allowed except > > I have largely used kernel multi-line comments in SWUpdate. Of course, > this is not an issue, but before we are going into a jungle of different > code-styles, we should stick to one of them. And then to: > > /* > * Multi line > * comments > */ Oops, I haven't noticed that yet and probably did all my patches wrong :) I'll try to consider this in my next patches... > > + * for downloader, webserver, and suricatta doing their own > > + * argv parsing. */ > > + c = '?'; > > + } > > switch (c) { > > case 'v': > > loglevel = TRACELEVEL; > > @@ -680,6 +686,12 @@ int main(int argc, char **argv) > > } > > } > > > > + if (optind < argc) { > > + /* SWUpdate has no non-option arguments, fail on them */ > > + usage(argv[0]); > > + exit(1); > > + } > > + > > /* > > * Parameters are parsed: now performs plausibility > > * tests before starting processes and threads > > @@ -698,6 +710,18 @@ int main(int argc, char **argv) > > exit(1); > > } > > > > +#ifdef CONFIG_SURICATTA > > + if (opt_u && (opt_c || opt_i > > +#ifdef CONFIG_DOWNLOAD > > + || opt_d > > +#endif > > + )) { > > + fprintf(stderr, "invalid mode combination with suricatta.\n"); > > + usage(argv[0]); > > + exit(1); > > + } > > +#endif > > I am not convinced this is required. First, it can be that one starts > SWUpdate in the same way, but in one case just adding "-c -i <file>". > And then starts SWUpdate with all options. Of course, SWUpdate exits, > but it does what it is supposed to do. Without this test, everything > works as expected: SWUpdate checks for a sw-description inside the file > provided with -i, parses and reports the result before exiting. Yes, and you gave SWUpdate parameters that aren't respected, i.e., they're ignored and hence superfluous. While this seems more convenient to some, I would wonder why the superfluous parameters were not respected and SWUpdate exits instead of, e.g., going to suricatta daemon mode in this example. I'd prefer SWUpdate not to silently not react to some parameters I gave it because of some "internal wirings". At least this wiring then has to be documented (in the docs as well as in --help). So, summing up, I think if I tell it what to do and this is an allowed parameter combination (has to be sensibly defined), it should do what I told it, all of it. But I guess this is a matter of personal taste and hence not really debatable :) That said, I'm fine with dropping this patch entirely or taking some parts of it as v2 as you like and which you like? > I can also think that -d and -u are not mutually exclusive, but just a > second server is available in case of Hawkbit failure (or viceversa). Yes, sure, this has to be adapted to the growing feature set of SWUpdate once these features are implemented. But this is true for almost all features: Once they're implemented you have to supply the "wiring" but in my opinion there's no sense in not doing, e.g., sanity checking in hindsight of potential features which aren't there yet. It simply has to be adapted when the features arrive. Kind regards, Christian
Hi Christian, On 22/08/2017 14:06, Christian Storm wrote: > Hi Stefano, > >>> (1) disallow options' values starting with '-' except for >>> downloader, webserver, and suricatta doing their own >>> cmdline parsing. Otherwise, e.g., this command >>> $ swupdate -l -c -i <file> >>> installs <file> instead of checking it due to -l's >>> option value missing. >>> (2) abort on superfluous non-option cmdline arguments >>> as SWUpdate doesn't use them, probably an usage error. >>> (3) check sensible combinations with suricatta mode >>> >>> Signed-off-by: Christian Storm <christian.storm@siemens.com> >>> --- >>> core/swupdate.c | 24 ++++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git a/core/swupdate.c b/core/swupdate.c >>> index b01aadd..f364bce 100644 >>> --- a/core/swupdate.c >>> +++ b/core/swupdate.c >>> @@ -599,6 +599,12 @@ int main(int argc, char **argv) >>> /* Process options with getopt */ >>> while ((c = getopt_long(argc, argv, main_options, >>> long_options, NULL)) != EOF) { >>> + if (optarg && *optarg == '-' && (c != 'd' && c != 'u' && c != 'w')) { >>> + /* An option's value starting with '-' is not allowed except >> >> I have largely used kernel multi-line comments in SWUpdate. Of course, >> this is not an issue, but before we are going into a jungle of different >> code-styles, we should stick to one of them. And then to: >> >> /* >> * Multi line >> * comments >> */ > > Oops, I haven't noticed that yet and probably did all my patches wrong :) > I'll try to consider this in my next patches... > Thanks ! >>> + * for downloader, webserver, and suricatta doing their own >>> + * argv parsing. */ >>> + c = '?'; >>> + } >>> switch (c) { >>> case 'v': >>> loglevel = TRACELEVEL; >>> @@ -680,6 +686,12 @@ int main(int argc, char **argv) >>> } >>> } >>> >>> + if (optind < argc) { >>> + /* SWUpdate has no non-option arguments, fail on them */ >>> + usage(argv[0]); >>> + exit(1); >>> + } >>> + >>> /* >>> * Parameters are parsed: now performs plausibility >>> * tests before starting processes and threads >>> @@ -698,6 +710,18 @@ int main(int argc, char **argv) >>> exit(1); >>> } >>> >>> +#ifdef CONFIG_SURICATTA >>> + if (opt_u && (opt_c || opt_i >>> +#ifdef CONFIG_DOWNLOAD >>> + || opt_d >>> +#endif >>> + )) { >>> + fprintf(stderr, "invalid mode combination with suricatta.\n"); >>> + usage(argv[0]); >>> + exit(1); >>> + } >>> +#endif >> >> I am not convinced this is required. First, it can be that one starts >> SWUpdate in the same way, but in one case just adding "-c -i <file>". >> And then starts SWUpdate with all options. Of course, SWUpdate exits, >> but it does what it is supposed to do. Without this test, everything >> works as expected: SWUpdate checks for a sw-description inside the file >> provided with -i, parses and reports the result before exiting. > > Yes, and you gave SWUpdate parameters that aren't respected, i.e., > they're ignored and hence superfluous. While this seems more convenient > to some, I would wonder why the superfluous parameters were not > respected and SWUpdate exits instead of, e.g., going to suricatta daemon > mode in this example. I'd prefer SWUpdate not to silently not react to > some parameters I gave it because of some "internal wirings". At least > this wiring then has to be documented (in the docs as well as in > --help). Here you are absolutely right ! > So, summing up, I think if I tell it what to do and this is an > allowed parameter combination (has to be sensibly defined), it should do > what I told it, all of it. > But I guess this is a matter of personal taste and hence not > really debatable :) Yes, and I do not know myself which is the best approach. To take care of your patch, you should at least drop the line : +#ifdef CONFIG_DOWNLOAD + || opt_d because this could be a use case. But I agree that -c / -i are mutually exclusive with suricatta. The other point I have seen when I tested your patch is: + fprintf(stderr, "invalid mode combination with suricatta.\n"); This is an error and should be noted very well. However, due to usage() output, the real reason is not so evident. I think it is better just to report the error (maybe with the note "run swupdate -h") without calling usage(). > That said, I'm fine with dropping this patch entirely or taking some > parts of it as v2 as you like and which you like? I am fine if you could rework this patch with the changes we have discussed :-) > > >> I can also think that -d and -u are not mutually exclusive, but just a >> second server is available in case of Hawkbit failure (or viceversa). > > Yes, sure, this has to be adapted to the growing feature set of SWUpdate > once these features are implemented. Right - but just for this case: downloader and hawkbit are already implemented. That means this can already be a use case. > But this is true for almost all > features: Once they're implemented you have to supply the "wiring" but > in my opinion there's no sense in not doing, Fully agree. > e.g., sanity checking in > hindsight of potential features which aren't there yet. It simply has to > be adapted when the features arrive. Fully agree, again. Best regards, Stefano
diff --git a/core/swupdate.c b/core/swupdate.c index b01aadd..f364bce 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -599,6 +599,12 @@ int main(int argc, char **argv) /* Process options with getopt */ while ((c = getopt_long(argc, argv, main_options, long_options, NULL)) != EOF) { + if (optarg && *optarg == '-' && (c != 'd' && c != 'u' && c != 'w')) { + /* An option's value starting with '-' is not allowed except + * for downloader, webserver, and suricatta doing their own + * argv parsing. */ + c = '?'; + } switch (c) { case 'v': loglevel = TRACELEVEL; @@ -680,6 +686,12 @@ int main(int argc, char **argv) } } + if (optind < argc) { + /* SWUpdate has no non-option arguments, fail on them */ + usage(argv[0]); + exit(1); + } + /* * Parameters are parsed: now performs plausibility * tests before starting processes and threads @@ -698,6 +710,18 @@ int main(int argc, char **argv) exit(1); } +#ifdef CONFIG_SURICATTA + if (opt_u && (opt_c || opt_i +#ifdef CONFIG_DOWNLOAD + || opt_d +#endif + )) { + fprintf(stderr, "invalid mode combination with suricatta.\n"); + usage(argv[0]); + exit(1); + } +#endif + swupdate_crypto_init(); if (strlen(swcfg.globals.publickeyfname)) {
(1) disallow options' values starting with '-' except for downloader, webserver, and suricatta doing their own cmdline parsing. Otherwise, e.g., this command $ swupdate -l -c -i <file> installs <file> instead of checking it due to -l's option value missing. (2) abort on superfluous non-option cmdline arguments as SWUpdate doesn't use them, probably an usage error. (3) check sensible combinations with suricatta mode Signed-off-by: Christian Storm <christian.storm@siemens.com> --- core/swupdate.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)