[pacman-dev] [PATCH v3 1/5] pacman-conf.c: accept short options
From: Ivy Foster <iff@escondida.tk> Signed-off-by: Ivy Foster <iff@escondida.tk> --- As per agregory's request in #archlinux-pacman, this version of the patch leaves the check for '?' well enough alone. '?' is getopt_long(3)'s way of indicating user error, and we can think of default: as indicating options forgotten in development. src/pacman/pacman-conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 1e6f55f9..3c9a80e2 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -59,7 +59,7 @@ static void parse_opts(int argc, char **argv) int c; config_file = CONFFILE; - const char *short_opts = ""; + const char *short_opts = "c:hlR:r:Vv"; struct option long_opts[] = { { "config" , required_argument , NULL , 'c' }, { "rootdir" , required_argument , NULL , 'R' }, -- 2.16.1
From: Ivy Foster <iff@escondida.tk> Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 3c9a80e2..9136b262 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -34,24 +34,21 @@ static void cleanup(void) config_free(config); } -static void usage(int ret) +static void usage(FILE *stream) { - FILE *stream = (ret ? stderr : stdout); -#define hputs(x) fputs(x"\n", stream) - hputs("pacman-conf - query pacman's configuration file"); - hputs("usage: pacman-conf [options] [<directive>...]"); - hputs(" pacman-conf (--repo-list|--help|--version)"); - hputs("options:"); - hputs(" --config=<path> set an alternate configuration file"); - hputs(" --rootdir=<path> set an alternate installation root"); - hputs(" --repo=<remote> query options for a specific repo"); - hputs(" --verbose always show directive names"); - hputs(" --repo-list list configured repositories"); - hputs(" --help display this help information"); - hputs(" --version display version information"); -#undef hputs - cleanup(); - exit(ret); + static const char help[] = + "pacman-conf: query pacman's configuration file\n" + "usage: pacman-conf [options] [directive]\n" + " pacman-conf [ { -l, --repo-list } | { -h, --help } | { -V, --version } ]\n" + "options:\n" + " -c, --config=<file> Read configuration from <file>\n" + " -h, --help Print help\n" + " -l, --repo-list List configured repositories\n" + " -r, --repo=<remote> Query options for a specific repo\n" + " -R, --rootdir=<dir> Use <dir> as system root\n" + " -v, --verbose Always show directive names\n" + " -V, --version Print version\n"; + fprintf(stream, "%s", help); } static void parse_opts(int argc, char **argv) @@ -89,8 +86,9 @@ static void parse_opts(int argc, char **argv) verbose = 1; break; case 'h': - usage(0); - break; + usage(stdout); + cleanup(); + exit(0); case 'V': printf("%s v%s\n", myname, myver); cleanup(); @@ -98,7 +96,9 @@ static void parse_opts(int argc, char **argv) break; case '?': default: - usage(1); + usage(stderr); + cleanup(); + exit(1); break; } } -- 2.16.1
On 02/10/18 at 10:15pm, iff@escondida.tk wrote:
From: Ivy Foster <iff@escondida.tk>
Signed-off-by: Ivy Foster <iff@escondida.tk> ---
There are two major changes here: adding short options to the usage and the "simplification" of the usage function. The short options should be added in the patch that made them available, not here. The other changes need some sort of justification. It's not obvious to me that this is simpler. Sure usage itself is a bit simpler, but now parse_opts has to worry about calling cleanup and exit. Other patches in this set similarly lack justification: using atexit and EXIT_SUCCESS/EXIT_FAILURE and limiting parse_opts to command-line options.
src/pacman/pacman-conf.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 3c9a80e2..9136b262 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -34,24 +34,21 @@ static void cleanup(void) config_free(config); }
-static void usage(int ret) +static void usage(FILE *stream) { - FILE *stream = (ret ? stderr : stdout); -#define hputs(x) fputs(x"\n", stream) - hputs("pacman-conf - query pacman's configuration file"); - hputs("usage: pacman-conf [options] [<directive>...]"); - hputs(" pacman-conf (--repo-list|--help|--version)"); - hputs("options:"); - hputs(" --config=<path> set an alternate configuration file"); - hputs(" --rootdir=<path> set an alternate installation root"); - hputs(" --repo=<remote> query options for a specific repo"); - hputs(" --verbose always show directive names"); - hputs(" --repo-list list configured repositories"); - hputs(" --help display this help information"); - hputs(" --version display version information"); -#undef hputs - cleanup(); - exit(ret); + static const char help[] = + "pacman-conf: query pacman's configuration file\n" + "usage: pacman-conf [options] [directive]\n" + " pacman-conf [ { -l, --repo-list } | { -h, --help } | { -V, --version } ]\n" + "options:\n" + " -c, --config=<file> Read configuration from <file>\n" + " -h, --help Print help\n" + " -l, --repo-list List configured repositories\n" + " -r, --repo=<remote> Query options for a specific repo\n" + " -R, --rootdir=<dir> Use <dir> as system root\n" + " -v, --verbose Always show directive names\n" + " -V, --version Print version\n"; + fprintf(stream, "%s", help); }
static void parse_opts(int argc, char **argv) @@ -89,8 +86,9 @@ static void parse_opts(int argc, char **argv) verbose = 1; break; case 'h': - usage(0); - break; + usage(stdout); + cleanup(); + exit(0); case 'V': printf("%s v%s\n", myname, myver); cleanup(); @@ -98,7 +96,9 @@ static void parse_opts(int argc, char **argv) break; case '?': default: - usage(1); + usage(stderr); + cleanup(); + exit(1); break; } } -- 2.16.1
From: Ivy Foster <iff@escondida.tk> This allows MYNAME to be used in place of the literal string "pacman-conf" in usage Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 9136b262..8985b326 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -21,7 +21,8 @@ #include <string.h> #include "conf.h" -const char *myname = "pacman-conf", *myver = "1.0.0"; +#define MYNAME "pacman-conf" +#define MYVER "1.0.0" alpm_list_t *directives = NULL; char sep = '\n', *repo_name = NULL; @@ -37,9 +38,9 @@ static void cleanup(void) static void usage(FILE *stream) { static const char help[] = - "pacman-conf: query pacman's configuration file\n" - "usage: pacman-conf [options] [directive]\n" - " pacman-conf [ { -l, --repo-list } | { -h, --help } | { -V, --version } ]\n" + MYNAME ": query pacman's configuration file\n" + "usage: " MYNAME " [options] [directive]\n" + " " MYNAME " [ { -l, --repo-list } | { -h, --help } | { -V, --version } ]\n" "options:\n" " -c, --config=<file> Read configuration from <file>\n" " -h, --help Print help\n" @@ -90,7 +91,7 @@ static void parse_opts(int argc, char **argv) cleanup(); exit(0); case 'V': - printf("%s v%s\n", myname, myver); + printf("%s v%s\n", MYNAME, MYVER); cleanup(); exit(0); break; -- 2.16.1
From: Ivy Foster <iff@escondida.tk> Since using atexit(3) draws in stdlib anyway and this changes every exit call at large and return call in main(), go ahead and use EXIT_SUCCESS and EXIt_FAILURE, too. Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 8985b326..51df8803 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -18,7 +18,9 @@ */ #include <getopt.h> +#include <stdlib.h> #include <string.h> + #include "conf.h" #define MYNAME "pacman-conf" @@ -88,19 +90,14 @@ static void parse_opts(int argc, char **argv) break; case 'h': usage(stdout); - cleanup(); - exit(0); + exit(EXIT_SUCCESS); case 'V': printf("%s v%s\n", MYNAME, MYVER); - cleanup(); - exit(0); - break; + exit(EXIT_SUCCESS); case '?': default: usage(stderr); - cleanup(); - exit(1); - break; + exit(EXIT_FAILURE); } } @@ -401,11 +398,15 @@ int main(int argc, char **argv) { int ret = 0; + if (atexit(cleanup) != 0) { + fputs("error: cannot set exit function\n", stderr); + return EXIT_FAILURE; + } + config = config_new(); parse_opts(argc, argv); if(!config) { - ret = 1; - goto cleanup; + return EXIT_FAILURE; } for(; optind < argc; optind++) { @@ -419,8 +420,7 @@ int main(int argc, char **argv) if(repo_list) { if(directives) { fputs("error: directives may not be specified with --repo-list\n", stderr); - ret = 1; - goto cleanup; + return EXIT_FAILURE; } list_repos(); } else if(repo_name) { @@ -429,10 +429,7 @@ int main(int argc, char **argv) ret = list_directives(); } -cleanup: - cleanup(); - - return ret; + return ret ? EXIT_FAILURE : EXIT_SUCCESS; } /* vim: set ts=2 sw=2 noet: */ -- 2.16.1
On 02/10/18 at 10:15pm, iff@escondida.tk wrote:
From: Ivy Foster <iff@escondida.tk>
Since using atexit(3) draws in stdlib anyway and this changes every exit call at large and return call in main(), go ahead and use EXIT_SUCCESS and EXIt_FAILURE, too.
Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 8985b326..51df8803 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -18,7 +18,9 @@ */
#include <getopt.h> +#include <stdlib.h> #include <string.h> + #include "conf.h"
#define MYNAME "pacman-conf" @@ -88,19 +90,14 @@ static void parse_opts(int argc, char **argv) break; case 'h': usage(stdout); - cleanup(); - exit(0); + exit(EXIT_SUCCESS); case 'V': printf("%s v%s\n", MYNAME, MYVER); - cleanup(); - exit(0); - break; + exit(EXIT_SUCCESS); case '?': default: usage(stderr); - cleanup(); - exit(1); - break; + exit(EXIT_FAILURE); } }
@@ -401,11 +398,15 @@ int main(int argc, char **argv) { int ret = 0;
+ if (atexit(cleanup) != 0) { + fputs("error: cannot set exit function\n", stderr); + return EXIT_FAILURE; + }
You're registering cleanup prior to initializing config. At this point in the patchset pacman-conf can't exit between the two, but a later patch delays the initialization, which can lead to a segfault.
config = config_new(); parse_opts(argc, argv); if(!config) { - ret = 1; - goto cleanup; + return EXIT_FAILURE; }
for(; optind < argc; optind++) { @@ -419,8 +420,7 @@ int main(int argc, char **argv) if(repo_list) { if(directives) { fputs("error: directives may not be specified with --repo-list\n", stderr); - ret = 1; - goto cleanup; + return EXIT_FAILURE; } list_repos(); } else if(repo_name) { @@ -429,10 +429,7 @@ int main(int argc, char **argv) ret = list_directives(); }
-cleanup: - cleanup(); - - return ret; + return ret ? EXIT_FAILURE : EXIT_SUCCESS; }
/* vim: set ts=2 sw=2 noet: */ -- 2.16.1
-- apg
From: Ivy Foster <iff@escondida.tk> Previously, it also parsed the config file instead of simply setting the (global) config_file variable Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 51df8803..4828dfec 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -100,10 +100,6 @@ static void parse_opts(int argc, char **argv) exit(EXIT_FAILURE); } } - - if(parseconfigfile(config_file) != 0 || setdefaults(config) != 0) { - fprintf(stderr, "error parsing '%s'\n", config_file); - } } static void list_repos(void) @@ -403,8 +399,16 @@ int main(int argc, char **argv) return EXIT_FAILURE; } - config = config_new(); parse_opts(argc, argv); + + if(!(config = config_new())) { + /* config_new prints the appropriate error message */ + return EXIT_FAILURE; + } + if(parseconfigfile(config_file) != 0 || setdefaults(config) != 0) { + fprintf(stderr, "error parsing '%s'\n", config_file); + return EXIT_FAILURE; + } if(!config) { return EXIT_FAILURE; } -- 2.16.1
On 02/10/18 at 10:15pm, iff@escondida.tk wrote:
From: Ivy Foster <iff@escondida.tk>
Previously, it also parsed the config file instead of simply setting the (global) config_file variable
I think the original purpose of this patch is fundamentally unnecessary, but it does (accidentally, I assume) fix a bug if parsing the config file fails.
Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 51df8803..4828dfec 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -100,10 +100,6 @@ static void parse_opts(int argc, char **argv) exit(EXIT_FAILURE); } } - - if(parseconfigfile(config_file) != 0 || setdefaults(config) != 0) { - fprintf(stderr, "error parsing '%s'\n", config_file); - } }
static void list_repos(void) @@ -403,8 +399,16 @@ int main(int argc, char **argv) return EXIT_FAILURE; }
- config = config_new(); parse_opts(argc, argv);
config must be initialized before parse_opts because parse_opts modifies it.
+ + if(!(config = config_new())) { + /* config_new prints the appropriate error message */ + return EXIT_FAILURE; + } + if(parseconfigfile(config_file) != 0 || setdefaults(config) != 0) { + fprintf(stderr, "error parsing '%s'\n", config_file); + return EXIT_FAILURE; + } if(!config) { return EXIT_FAILURE; } -- 2.16.1
participants (2)
-
Andrew Gregory
-
iff＠escondida.tk