[pacman-dev] [PATCH 1/6] pacman.c: remove unnecessary optarg checks
getopt takes care of making sure that options that require a value have one. These checks were only added to silence clang, which no longer complains about optarg being unchecked, and newer options already use optarg unchecked. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- I haven't been able to find a warning flag that makes clang complain anyway. src/pacman/pacman.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 6bf94e9..1fb447c 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -321,8 +321,6 @@ static void handler(int signum) cleanup(128 + signum); } -#define check_optarg() if(!optarg) { return 1; } - static void invalid_opt(int used, const char *opt1, const char *opt2) { if(used) { @@ -337,7 +335,6 @@ static int parsearg_util_addlist(alpm_list_t **list) { char *i, *save; - check_optarg(); for(i = strtok_r(optarg, ",", &save); i; i = strtok_r(NULL, ",", &save)) { *list = alpm_list_add(*list, strdup(i)); @@ -393,16 +390,13 @@ static int parsearg_global(int opt) { switch(opt) { case OP_ARCH: - check_optarg(); config_set_arch(optarg); break; case OP_ASK: - check_optarg(); config->noask = 1; config->ask = (unsigned int)atoi(optarg); break; case OP_CACHEDIR: - check_optarg(); config->cachedirs = alpm_list_add(config->cachedirs, strdup(optarg)); break; case OP_COLOR: @@ -420,7 +414,6 @@ static int parsearg_global(int opt) enable_colors(config->color); break; case OP_CONFIG: - check_optarg(); if(config->configfile) { free(config->configfile); } @@ -453,7 +446,6 @@ static int parsearg_global(int opt) config->gpgdir = strdup(optarg); break; case OP_LOGFILE: - check_optarg(); config->logfile = strndup(optarg, PATH_MAX); break; case OP_NOCONFIRM: @@ -461,12 +453,10 @@ static int parsearg_global(int opt) break; case OP_DBPATH: case 'b': - check_optarg(); config->dbpath = strdup(optarg); break; case OP_ROOT: case 'r': - check_optarg(); config->rootdir = strdup(optarg); break; case OP_VERBOSE: @@ -632,7 +622,6 @@ static int parsearg_trans(int opt) config->print = 1; break; case OP_PRINTFORMAT: - check_optarg(); config->print = 1; config->print_format = strdup(optarg); break; -- 1.8.5.2
Plugs a memory leak when values were passed twice. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/pacman.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 1fb447c..748bc54 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -443,9 +443,11 @@ static int parsearg_global(int opt) config->noprogressbar = 1; break; case OP_GPGDIR: + free(config->gpgdir); config->gpgdir = strdup(optarg); break; case OP_LOGFILE: + free(config->logfile); config->logfile = strndup(optarg, PATH_MAX); break; case OP_NOCONFIRM: @@ -453,10 +455,12 @@ static int parsearg_global(int opt) break; case OP_DBPATH: case 'b': + free(config->dbpath); config->dbpath = strdup(optarg); break; case OP_ROOT: case 'r': + free(config->rootdir); config->rootdir = strdup(optarg); break; case OP_VERBOSE: @@ -623,6 +627,7 @@ static int parsearg_trans(int opt) break; case OP_PRINTFORMAT: config->print = 1; + free(config->print_format); config->print_format = strdup(optarg); break; default: -- 1.8.5.2
On 14/01/14 14:01, Andrew Gregory wrote:
Plugs a memory leak when values were passed twice.
I'd much prefer we aborted instead. Specifying options twice is a sign of an error on the users behalf. Allan
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/pacman.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 1fb447c..748bc54 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -443,9 +443,11 @@ static int parsearg_global(int opt) config->noprogressbar = 1; break; case OP_GPGDIR: + free(config->gpgdir); config->gpgdir = strdup(optarg); break; case OP_LOGFILE: + free(config->logfile); config->logfile = strndup(optarg, PATH_MAX); break; case OP_NOCONFIRM: @@ -453,10 +455,12 @@ static int parsearg_global(int opt) break; case OP_DBPATH: case 'b': + free(config->dbpath); config->dbpath = strdup(optarg); break; case OP_ROOT: case 'r': + free(config->rootdir); config->rootdir = strdup(optarg); break; case OP_VERBOSE: @@ -623,6 +627,7 @@ static int parsearg_trans(int opt) break; case OP_PRINTFORMAT: config->print = 1; + free(config->print_format); config->print_format = strdup(optarg); break; default:
I can't think of another command line application that has that behaviour on parsing command line arguments.
Yes, please don't do that, it breaks alias chains. J. Leclanche On Wed, Jan 15, 2014 at 4:08 PM, Simon Gomizelj <simongmzlj@gmail.com> wrote:
I can't think of another command line application that has that behaviour on parsing command line arguments.
The user requesting usage or version information is not an error. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/pacman.c | 4 ++-- test/pacman/tests/pacman001.py | 2 +- test/pacman/tests/pacman002.py | 2 +- test/pacman/tests/pacman003.py | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 748bc54..d7072d2 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -889,11 +889,11 @@ static int parseargs(int argc, char *argv[]) } if(config->help) { usage(config->op, mbasename(argv[0])); - return 2; + cleanup(0); } if(config->version) { version(); - return 2; + cleanup(0); } /* parse all other options */ diff --git a/test/pacman/tests/pacman001.py b/test/pacman/tests/pacman001.py index d467e3f..1d3a36a 100644 --- a/test/pacman/tests/pacman001.py +++ b/test/pacman/tests/pacman001.py @@ -2,4 +2,4 @@ self.args = "--version" -self.addrule("PACMAN_RETCODE=2") +self.addrule("PACMAN_RETCODE=0") diff --git a/test/pacman/tests/pacman002.py b/test/pacman/tests/pacman002.py index c021725..2add614 100644 --- a/test/pacman/tests/pacman002.py +++ b/test/pacman/tests/pacman002.py @@ -2,4 +2,4 @@ self.args = "--help" -self.addrule("PACMAN_RETCODE=2") +self.addrule("PACMAN_RETCODE=0") diff --git a/test/pacman/tests/pacman003.py b/test/pacman/tests/pacman003.py index b527594..a80e2d6 100644 --- a/test/pacman/tests/pacman003.py +++ b/test/pacman/tests/pacman003.py @@ -2,4 +2,4 @@ self.args = "-S --help" -self.addrule("PACMAN_RETCODE=2") +self.addrule("PACMAN_RETCODE=0") -- 1.8.5.2
On Mon, Jan 13, 2014 at 10:01 PM, Andrew Gregory <andrew.gregory.8@gmail.com
wrote:
The user requesting usage or version information is not an error.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
$ git grep 'return 2;' src/ | cat src/pacman/pacman.c: return 2; src/pacman/pacman.c: return 2; src/util/pacsort.c: return 2; src/util/vercmp.c: return 2; I'd broaden the scope of your patch a bit; might want to check the non-C utilities too. -Dan
The user requesting usage or version information is not an error. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- Expanded patch to include pacsort. All other utilities already exit 0. src/pacman/pacman.c | 4 ++-- src/util/pacsort.c | 9 ++++++++- test/pacman/tests/pacman001.py | 2 +- test/pacman/tests/pacman002.py | 2 +- test/pacman/tests/pacman003.py | 2 +- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7144339..8c76987 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -889,11 +889,11 @@ static int parseargs(int argc, char *argv[]) } if(config->help) { usage(config->op, mbasename(argv[0])); - return 2; + cleanup(0); } if(config->version) { version(); - return 2; + cleanup(0); } /* parse all other options */ diff --git a/src/util/pacsort.c b/src/util/pacsort.c index 948b03d..2d53a1c 100644 --- a/src/util/pacsort.c +++ b/src/util/pacsort.c @@ -45,6 +45,7 @@ static struct options_t { int sortkey; int null; int filemode; + int help; char delim; } opts; @@ -374,7 +375,8 @@ static int parse_options(int argc, char **argv) opts.filemode = 1; break; case 'h': - return 1; + opts.help = 1; + return 0; case 'k': opts.sortkey = (int)strtol(optarg, NULL, 10); if(opts.sortkey <= 0) { @@ -420,6 +422,11 @@ int main(int argc, char *argv[]) return 2; } + if(opts.help) { + usage(); + return 0; + } + list = list_new(100); buffer = buffer_new(BUFSIZ * 3); diff --git a/test/pacman/tests/pacman001.py b/test/pacman/tests/pacman001.py index d467e3f..1d3a36a 100644 --- a/test/pacman/tests/pacman001.py +++ b/test/pacman/tests/pacman001.py @@ -2,4 +2,4 @@ self.args = "--version" -self.addrule("PACMAN_RETCODE=2") +self.addrule("PACMAN_RETCODE=0") diff --git a/test/pacman/tests/pacman002.py b/test/pacman/tests/pacman002.py index c021725..2add614 100644 --- a/test/pacman/tests/pacman002.py +++ b/test/pacman/tests/pacman002.py @@ -2,4 +2,4 @@ self.args = "--help" -self.addrule("PACMAN_RETCODE=2") +self.addrule("PACMAN_RETCODE=0") diff --git a/test/pacman/tests/pacman003.py b/test/pacman/tests/pacman003.py index b527594..a80e2d6 100644 --- a/test/pacman/tests/pacman003.py +++ b/test/pacman/tests/pacman003.py @@ -2,4 +2,4 @@ self.args = "-S --help" -self.addrule("PACMAN_RETCODE=2") +self.addrule("PACMAN_RETCODE=0") -- 1.8.5.3
On 29/01/14 12:40, Andrew Gregory wrote:
The user requesting usage or version information is not an error.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com>
Ack.
We only care that packages are being installed from a repo, not how many. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/sync.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 4ae01ac..468d760 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -371,7 +371,7 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) alpm_list_t *i, *j; alpm_list_t *deps = NULL; alpm_list_t *unresolvable = NULL; - size_t from_sync = 0; + int from_sync = 0; int ret = 0; alpm_trans_t *trans = handle->trans; @@ -381,7 +381,10 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) for(i = trans->add; i; i = i->next) { alpm_pkg_t *spkg = i->data; - from_sync += (spkg->origin == ALPM_PKG_FROM_SYNCDB); + if (spkg->origin == ALPM_PKG_FROM_SYNCDB){ + from_sync = 1; + break; + } } /* ensure all sync database are valid if we will be using them */ -- 1.8.5.2
Commit e47eb9a7 commented out base64_encode, which left base64_enc_map unused, causing warnings under clang. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/base64.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/libalpm/base64.c b/lib/libalpm/base64.c index 32c4445..31ecca1 100644 --- a/lib/libalpm/base64.c +++ b/lib/libalpm/base64.c @@ -36,6 +36,7 @@ #include "base64.h" +#if 0 static const unsigned char base64_enc_map[64] = { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', @@ -46,6 +47,7 @@ static const unsigned char base64_enc_map[64] = 'y', 'z', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '+', '/' }; +#endif static const unsigned char base64_dec_map[128] = { -- 1.8.5.2
Now, I'm not a dev, but this piqued my curiosity. Since this code is under version control, why comment it out instead of just deleting it? On Tue, Jan 14, 2014 at 1:01 PM, Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
Commit e47eb9a7 commented out base64_encode, which left base64_enc_map unused, causing warnings under clang.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/base64.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/lib/libalpm/base64.c b/lib/libalpm/base64.c index 32c4445..31ecca1 100644 --- a/lib/libalpm/base64.c +++ b/lib/libalpm/base64.c @@ -36,6 +36,7 @@
#include "base64.h"
+#if 0 static const unsigned char base64_enc_map[64] = { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', @@ -46,6 +47,7 @@ static const unsigned char base64_enc_map[64] = 'y', 'z', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '+', '/' }; +#endif
static const unsigned char base64_dec_map[128] = { -- 1.8.5.2
On 14/01/14 18:16, Emil Lundberg wrote:
Now, I'm not a dev, but this piqued my curiosity. Since this code is under version control, why comment it out instead of just deleting it?
This file is taken from another source. Just committing it out will prevent us pulling the code back in when where merge an upstream update. A
It's a boolean, so signedness doesn't matter, and the public API already exposes it as an int through alpm_pkg_has_scriptlet(). Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/package.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/libalpm/package.h b/lib/libalpm/package.h index 228fca3..a972d12 100644 --- a/lib/libalpm/package.h +++ b/lib/libalpm/package.h @@ -131,7 +131,7 @@ struct __alpm_pkg_t { alpm_pkgvalidation_t validation; alpm_pkgfrom_t origin; alpm_pkgreason_t reason; - unsigned int scriptlet; + int scriptlet; }; alpm_file_t *_alpm_file_copy(alpm_file_t *dest, const alpm_file_t *src); -- 1.8.5.2
On Mon, Jan 13, 2014 at 10:01 PM, Andrew Gregory <andrew.gregory.8@gmail.com
wrote:
It's a boolean, so signedness doesn't matter, and the public API already exposes it as an int through alpm_pkg_has_scriptlet().
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
This snuck in on my last patch set, whoops.
lib/libalpm/package.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/libalpm/package.h b/lib/libalpm/package.h index 228fca3..a972d12 100644 --- a/lib/libalpm/package.h +++ b/lib/libalpm/package.h @@ -131,7 +131,7 @@ struct __alpm_pkg_t { alpm_pkgvalidation_t validation; alpm_pkgfrom_t origin; alpm_pkgreason_t reason; - unsigned int scriptlet; + int scriptlet; };
alpm_file_t *_alpm_file_copy(alpm_file_t *dest, const alpm_file_t *src); -- 1.8.5.2
On 14/01/14 14:01, Andrew Gregory wrote:
getopt takes care of making sure that options that require a value have one. These checks were only added to silence clang, which no longer complains about optarg being unchecked, and newer options already use optarg unchecked.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
I haven't been able to find a warning flag that makes clang complain anyway.
These were added in commit b8b8c786 at the end of 2009. I'd bet this stopped used without initialization warnings. That would be in the days of clang 2.6. I'm fine with removing this given the gpgdir config option uses optarg without this check, so the clang issue must have been fixed a while ago. Allan
src/pacman/pacman.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 6bf94e9..1fb447c 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -321,8 +321,6 @@ static void handler(int signum) cleanup(128 + signum); }
-#define check_optarg() if(!optarg) { return 1; } - static void invalid_opt(int used, const char *opt1, const char *opt2) { if(used) { @@ -337,7 +335,6 @@ static int parsearg_util_addlist(alpm_list_t **list) { char *i, *save;
- check_optarg();
for(i = strtok_r(optarg, ",", &save); i; i = strtok_r(NULL, ",", &save)) { *list = alpm_list_add(*list, strdup(i)); @@ -393,16 +390,13 @@ static int parsearg_global(int opt) { switch(opt) { case OP_ARCH: - check_optarg(); config_set_arch(optarg); break; case OP_ASK: - check_optarg(); config->noask = 1; config->ask = (unsigned int)atoi(optarg); break; case OP_CACHEDIR: - check_optarg(); config->cachedirs = alpm_list_add(config->cachedirs, strdup(optarg)); break; case OP_COLOR: @@ -420,7 +414,6 @@ static int parsearg_global(int opt) enable_colors(config->color); break; case OP_CONFIG: - check_optarg(); if(config->configfile) { free(config->configfile); } @@ -453,7 +446,6 @@ static int parsearg_global(int opt) config->gpgdir = strdup(optarg); break; case OP_LOGFILE: - check_optarg(); config->logfile = strndup(optarg, PATH_MAX); break; case OP_NOCONFIRM: @@ -461,12 +453,10 @@ static int parsearg_global(int opt) break; case OP_DBPATH: case 'b': - check_optarg(); config->dbpath = strdup(optarg); break; case OP_ROOT: case 'r': - check_optarg(); config->rootdir = strdup(optarg); break; case OP_VERBOSE: @@ -632,7 +622,6 @@ static int parsearg_trans(int opt) config->print = 1; break; case OP_PRINTFORMAT: - check_optarg(); config->print = 1; config->print_format = strdup(optarg); break;
participants (6)
-
Allan McRae
-
Andrew Gregory
-
Dan McGee
-
Emil Lundberg
-
Jerome Leclanche
-
Simon Gomizelj