[pacman-dev] [PATCH 1/6] pacman.c: remove unnecessary optarg checks

newer
[pacman-dev] [PATCH 0/4] Smatch...

older
[pacman-dev] [PATCH] Consistently...

Andrew Gregory

14 Jan 2014 14 Jan '14

4:01 a.m.

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 --- 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

Show replies by date

Andrew Gregory

14 Jan 14 Jan

4:01 a.m.

New subject: [pacman-dev] [PATCH 2/6] pacman.c: free string commandline options

Plugs a memory leak when values were passed twice. Signed-off-by: Andrew Gregory --- 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

Allan McRae

15 Jan 15 Jan

7:07 a.m.

New subject: [pacman-dev] [PATCH 2/6] pacman.c: free string commandline options

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 --- 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:

Simon Gomizelj

4:08 p.m.

New subject: [pacman-dev] [PATCH 2/6] pacman.c: free string commandline options

I can't think of another command line application that has that behaviour on parsing command line arguments.

Jerome Leclanche

4:21 p.m.

New subject: [pacman-dev] [PATCH 2/6] pacman.c: free string commandline options

Yes, please don't do that, it breaks alias chains. J. Leclanche On Wed, Jan 15, 2014 at 4:08 PM, Simon Gomizelj wrote:

...

I can't think of another command line application that has that behaviour on parsing command line arguments.

Andrew Gregory

14 Jan 14 Jan

4:01 a.m.

New subject: [pacman-dev] [PATCH 3/6] pacman: use exit status 0 for --help and --version

The user requesting usage or version information is not an error. Signed-off-by: Andrew Gregory --- 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

Dan McGee

15 Jan 15 Jan

2:25 p.m.

New subject: [pacman-dev] [PATCH 3/6] pacman: use exit status 0 for --help and --version

On Mon, Jan 13, 2014 at 10:01 PM, Andrew Gregory

...

wrote:

...

The user requesting usage or version information is not an error.

Signed-off-by: Andrew Gregory ---

$ 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

Andrew Gregory

29 Jan 29 Jan

2:40 a.m.

New subject: [pacman-dev] [PATCHv2] pacman: use exit status 0 for --help and --version

The user requesting usage or version information is not an error. Signed-off-by: Andrew Gregory --- 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

Allan McRae

30 Jan 30 Jan

3:12 a.m.

New subject: [pacman-dev] [PATCHv2] pacman: use exit status 0 for --help and --version

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

Ack.

Andrew Gregory

14 Jan 14 Jan

4:01 a.m.

New subject: [pacman-dev] [PATCH 4/6] sync_prepare: treat from_sync as a boolean

We only care that packages are being installed from a repo, not how many. Signed-off-by: Andrew Gregory --- 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

Andrew Gregory

4:01 a.m.

New subject: [pacman-dev] [PATCH 5/6] base64.c: comment out unused variable

Commit e47eb9a7 commented out base64_encode, which left base64_enc_map unused, causing warnings under clang. Signed-off-by: Andrew Gregory --- 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

Emil Lundberg

8:16 a.m.

New subject: [pacman-dev] [PATCH 5/6] base64.c: comment out unused variable

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 wrote:

...

Commit e47eb9a7 commented out base64_encode, which left base64_enc_map unused, causing warnings under clang.

Signed-off-by: Andrew Gregory --- 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

Allan McRae

8:56 a.m.

New subject: [pacman-dev] [PATCH 5/6] base64.c: comment out unused variable

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

Andrew Gregory

4:01 a.m.

New subject: [pacman-dev] [PATCH 6/6] package.h: convert scriptlet field to int

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 --- 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

Dan McGee

1:09 p.m.

New subject: [pacman-dev] [PATCH 6/6] package.h: convert scriptlet field to int

On Mon, Jan 13, 2014 at 10:01 PM, Andrew Gregory

...

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 ---

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

Allan McRae

15 Jan 15 Jan

7:05 a.m.

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 ---

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;

3754

Age (days ago)

3770

Last active (days ago)

List overview

Download

15 comments

6 participants

participants (6)

Allan McRae
Andrew Gregory
Dan McGee
Emil Lundberg
Jerome Leclanche
Simon Gomizelj