Re: Any interest in eliminating sprintf(), strcpy(), and strcat()?
On Mar 9, 2008, at 12:26 AM, Wayne Davison wrote:
On Sat, Mar 08, 2008 at 10:42:36AM -0800, Wayne Davison wrote:
(i.e. -1 could be mapped to the limit-value+1 without need to compute
the real overflow length because popt doesn't ever call snprintf()
expecting to find out how much bigger its buffer needs to be
This is apparently no longer true. The strdup_vprintf() function
expects to get a valid length back after calling with a limit
length of
1, so this code is currently broken on systems that return -1 for
overflow, and there is a variable overflow of the "char c" stack
variable on systems that don't count the null in the limit.
So, it looks like you need to fix that before releasing 1.14. The
variable overflow can be easily fixed by making c a 2-character array,
(or perhaps passing a 0 limit to snprintf() instead of 1). Avoiding a
return of -1 could be fixed by substituting a working snprintf()
function, if you want to go to that extreme. Rsync does this using
this
code:
http://rsync.samba.org/ftp/unpacked/rsync/lib/snprintf.c
For rsync, I just use asprintf() to get an allocated string from a
printf() format, and that has been quite portable in all the various
systems that rsync runs on (including various flavors of Unix, Linux,
and Cygwin). I don't know if you ran into a compatibility problem
that
made you want to avoid it, however.
Nah, I'm way not happy with the complexity introduced by undertaking
UTF-8 conversion on the fly in popt --help.
The va_copy boogered the popt-1.11 release, and the crapola _STILL_
isn't correct
or functional 3 releases later.
Meanwhile, what do you think of this patch:
Index: poptint.c
===
RCS file: /v/rpm/cvs/popt/poptint.c,v
retrieving revision 1.16
diff -u -b -B -w -p -r1.16 poptint.c
--- poptint.c 17 Feb 2008 00:53:49 - 1.16
+++ poptint.c 9 Mar 2008 06:13:42 -
@@ -129,7 +129,7 @@ strdup_vprintf (const char *format, va_l
/[EMAIL PROTECTED] ap @*/
{
char *buffer;
-char c;
+char c[2];
va_list apc;
int xx;
@@ -137,7 +137,7 @@ strdup_vprintf (const char *format, va_l
va_copy(apc, ap); /* XXX linux amd64/ppc needs a copy. */
/[EMAIL PROTECTED] =unrecog @*/
-buffer = calloc(sizeof(*buffer), (size_t)vsnprintf (&c, (size_t)
1, format, ap) + 1);
+buffer = calloc(sizeof(*buffer), (size_t)vsnprintf (&c[0],
(size_t)1, format, ap) + 2);
if (buffer != NULL)
xx = vsprintf(buffer, format, apc);
73 de Jeff
__
POPT Library http://rpm5.org
Developer Communication List popt-devel@rpm5.org
Re: Allow equal after a short option
On Mar 8, 2008, at 11:24 PM, Wayne Davison wrote: On Sat, Mar 08, 2008 at 06:11:09PM -0500, Jeff Johnson wrote: Hmmm, we appear to have different behavior wrto echo. Your patch changes testit.sh to include an explicit "--", which (when I last fixed testit.sh like 3 weeks ago) does not appear in the output I am (and was) seeing. I tried it on Ubuntu 7.10 and CentOS 5 with the same result, so it's obviously a difference between whatever version of "echo" you have and the one in the gnu coreutils package. I'll attach a patch that makes the code use a simple perl -e construct to accomplish the same thing in a compatible manner (for any system with perl). Using that avoids the need to add the "--" chars to the output like I did in my earlier patch. While I believe you, I need to pass "make check" before releasing popt-1.14 or enabling your patch. /bin/echo on my system is unmodified from Fedora 9 coreutils-6.10-4.fc9.i386, verified with rpm -Vf /bin/echo. Claiming "obviously" explains nothing. And I'd rather understand the problem than band-aid with perl. Using perl in a popt "make check" adds a difficult build prerequsite as well. popt needs to handle "--" like any other CLI argument. I have added the tests and the 1 liner to have -c=foo functionality, just commented out and disabled for now. Please note that that one-line fix won't work without my prior patch that fixes the problem with a short option that has an embedded (or leading) equal in an abutting arg (e.g. "test1 -2foo=bar"). I'll attach it here in case you missed it. I added the longArg = NULL, am seeing the same failure on test # 9. If I skip test #9, there's several other tests that fail. The one element that changes is enabling/disabling your patch. And its not just /bin/echo that breaks. Here is rpm behavior with your -c=foo 1-liner enabled: [EMAIL PROTECTED] popt]$ rpm -q coreutils coreutils rpm: -q: unknown option Reverting the change, rebuilding, installing, I get this behavior instead: $ rpm -q coreutils coreutils-6.10-4.fc9.i386 So there's something not correct with your patch ... 73 de Jeff __ POPT Library http://rpm5.org Developer Communication List popt-devel@rpm5.org
Re: Any interest in eliminating sprintf(), strcpy(), and strcat()?
On Sat, Mar 08, 2008 at 10:42:36AM -0800, Wayne Davison wrote: > (i.e. -1 could be mapped to the limit-value+1 without need to compute > the real overflow length because popt doesn't ever call snprintf() > expecting to find out how much bigger its buffer needs to be This is apparently no longer true. The strdup_vprintf() function expects to get a valid length back after calling with a limit length of 1, so this code is currently broken on systems that return -1 for overflow, and there is a variable overflow of the "char c" stack variable on systems that don't count the null in the limit. So, it looks like you need to fix that before releasing 1.14. The variable overflow can be easily fixed by making c a 2-character array, (or perhaps passing a 0 limit to snprintf() instead of 1). Avoiding a return of -1 could be fixed by substituting a working snprintf() function, if you want to go to that extreme. Rsync does this using this code: http://rsync.samba.org/ftp/unpacked/rsync/lib/snprintf.c For rsync, I just use asprintf() to get an allocated string from a printf() format, and that has been quite portable in all the various systems that rsync runs on (including various flavors of Unix, Linux, and Cygwin). I don't know if you ran into a compatibility problem that made you want to avoid it, however. ..wayne.. __ POPT Library http://rpm5.org Developer Communication List popt-devel@rpm5.org
portability: _ABS() and DBL_EPSILON
There are some UNIX systems that have their own _ABS() define in the
system header files, so the redefinition of _ABS() in popt.c can cause
problems. I changed the define to be POPT_ABS(). Also, some systems
don't seem to have DBL_EPSILON, so I define that if it is missing.
See attached patch.
..wayne..
--- popt.c 8 Mar 2008 17:26:30 - 1.116
+++ popt.c 9 Mar 2008 03:48:43 -
@@ -18,6 +18,10 @@
#include "findme.h"
#include "poptint.h"
+#ifndef DBL_EPSILON
+#define DBL_EPSILON 2.2204460492503131e-16
+#endif
+
#ifdef MYDEBUG
/[EMAIL PROTECTED]@*/
int _popt_debug = 0;
@@ -1075,10 +1079,10 @@ int poptGetNextOpt(poptContext con)
if (poptArgType(opt) == POPT_ARG_DOUBLE) {
arg.doublep[0] = aDouble;
} else {
-#define _ABS(a)a) - 0.0) < DBL_EPSILON) ? -(a) : (a))
- if ((_ABS(aDouble) - FLT_MAX) > DBL_EPSILON)
+#define POPT_ABS(a) a) - 0.0) < DBL_EPSILON) ? -(a) : (a))
+ if ((POPT_ABS(aDouble) - FLT_MAX) > DBL_EPSILON)
return POPT_ERROR_OVERFLOW;
- if ((FLT_MIN - _ABS(aDouble)) > DBL_EPSILON)
+ if ((FLT_MIN - POPT_ABS(aDouble)) > DBL_EPSILON)
return POPT_ERROR_OVERFLOW;
arg.floatp[0] = (float) aDouble;
}
Re: Allow equal after a short option
On Sat, Mar 08, 2008 at 06:11:09PM -0500, Jeff Johnson wrote:
> Hmmm, we appear to have different behavior wrto echo. Your
> patch changes testit.sh to include an explicit "--", which (when
> I last fixed testit.sh like 3 weeks ago) does not appear in the
> output I am (and was) seeing.
I tried it on Ubuntu 7.10 and CentOS 5 with the same result, so it's
obviously a difference between whatever version of "echo" you have and
the one in the gnu coreutils package. I'll attach a patch that makes
the code use a simple perl -e construct to accomplish the same thing in
a compatible manner (for any system with perl). Using that avoids the
need to add the "--" chars to the output like I did in my earlier patch.
> I have added the tests and the 1 liner to have -c=foo functionality,
> just commented out and disabled for now.
Please note that that one-line fix won't work without my prior patch
that fixes the problem with a short option that has an embedded (or
leading) equal in an abutting arg (e.g. "test1 -2foo=bar"). I'll attach
it here in case you missed it.
..wayne..
--- test-poptrc 16 Feb 2008 22:16:10 - 1.4
+++ test-poptrc 9 Mar 2008 04:13:55 -
@@ -7,6 +7,6 @@ test1 alias -O --arg1
test1 alias --grab --arg2 "'foo !#:+'"
test1 alias --grabbar --grab bar
-test1 exec --echo-args echo --
+test1 exec --echo-args perl -e 'print "@ARGV"' --
test1 alias -e --echo-args
-test1 exec -a /bin/echo --
+test1 exec -a perl -e 'print "@ARGV"' --
--- popt.c 8 Mar 2008 17:26:30 - 1.116
+++ popt.c 8 Mar 2008 20:48:43 -
@@ -901,6 +901,7 @@ int poptGetNextOpt(poptContext con)
if (!opt) {
con->os->nextCharArg = origOptString + 1;
+ longArg = NULL;
} else {
if (con->os == con->optionStack && F_ISSET(opt, STRIP))
{
Re: Allow equal after a short option
On Mar 8, 2008, at 12:56 PM, Wayne Davison wrote: On Sat, Mar 08, 2008 at 12:10:52PM -0500, Jeff Johnson wrote: Test "test1 -2 foo" failed with: "arg1: 0 arg2: rest: foo" != "arg1: 0 arg2: foo" I'm not seeing that error with the CVS version. I do note that my prior patch to fix the longArg pointer (e.g. "./test1 -2foo=bar") isn't there, but even commenting out that fix doesn't get test 9 to fail for me. I did see failures for tests 19-23 due to the --echo-args macro now outputting an extra "--" at the start. The attached patch fixes this, and also adds two new tests to check that the changed equal-handling works right. Hmmm, we appear to have different behavior wrto echo. Your patch changes testit.sh to include an explicit "--", which (when I last fixed testit.sh like 3 weeks ago) does not appear in the output I am (and was) seeing. I need a functional testit.sh, so I'm gonna wait for popt-1.14 release before wrestling further with the contextual problems of --echo-args behavior. I have added the tests and the 1 liner to have -c=foo functionality, just commented out and disabled for now. 73 de Jeff __ POPT Library http://rpm5.org Developer Communication List popt-devel@rpm5.org
Re: Allow equal after a short option
On Mar 8, 2008, at 12:56 PM, Wayne Davison wrote: On Sat, Mar 08, 2008 at 12:10:52PM -0500, Jeff Johnson wrote: Test "test1 -2 foo" failed with: "arg1: 0 arg2: rest: foo" != "arg1: 0 arg2: foo" I'm not seeing that error with the CVS version. I do note that my prior patch to fix the longArg pointer (e.g. "./test1 -2foo=bar") isn't there, but even commenting out that fix doesn't get test 9 to fail for me. I did see failures for tests 19-23 due to the --echo-args macro now outputting an extra "--" at the start. The attached patch fixes this, and also adds two new tests to check that the changed equal-handling works right. Those tests were run from a freshly built CVS from HEAD. But perhaps I've tricked myself somehow. Lemme study a bit while merging in this patch. Hmmm, I was seeing exactly the opposite, --echo-args was failing until I added the "--" argument separator a couple of weeks ago. 73 de Jeff __ POPT Library http://rpm5.org Developer Communication List popt-devel@rpm5.org
Re: Any interest in eliminating sprintf(), strcpy(), and strcat()?
On Mar 8, 2008, at 1:42 PM, Wayne Davison wrote: In rsync I eliminated all use of sprintf(), strcpy(), and strcat(), replacing them with snprintf(), strlcpy(), and strlcat(). Would you be interested in such changes if appropriate compatibility functions were defined? For instance, I could imagine a configure test to see if snprintf() returns a -1 on overflow, and a test to see if its limit is off by 1 (some don't count the '\0'). Then, a simple wrapper function would handle both those conditions in a simple way (i.e. -1 could be mapped to the limit-value+1 without need to compute the real overflow length because popt doesn't ever call snprintf() expecting to find out how much bigger its buffer needs to be -- the limit would just be used to be extra sure that overflow was impossible. I have compatibility functions for strlcpy() and strlcat() that I could snag from rsync. These functions are better than strncpy() and strncat() as their limit value is the size of the buffer (unlike strncat()), and the destination string will always be '\0'-terminated (unlike strncpy()). I have most of the changes written for an earlier version (rsync includes a version of 1.10.2 at present) that I could adapt for 1.14. I can take a look, and am not averse to changing, but even snprintf has portability problems wrto popt, and strlfoo is likely even more problematic. There's *LOTS* of popt use around, so I tend to track with least- common-denominator as much as possible. 73 de Jeff __ POPT Library http://rpm5.org Developer Communication List popt-devel@rpm5.org
Any interest in eliminating sprintf(), strcpy(), and strcat()?
In rsync I eliminated all use of sprintf(), strcpy(), and strcat(), replacing them with snprintf(), strlcpy(), and strlcat(). Would you be interested in such changes if appropriate compatibility functions were defined? For instance, I could imagine a configure test to see if snprintf() returns a -1 on overflow, and a test to see if its limit is off by 1 (some don't count the '\0'). Then, a simple wrapper function would handle both those conditions in a simple way (i.e. -1 could be mapped to the limit-value+1 without need to compute the real overflow length because popt doesn't ever call snprintf() expecting to find out how much bigger its buffer needs to be -- the limit would just be used to be extra sure that overflow was impossible. I have compatibility functions for strlcpy() and strlcat() that I could snag from rsync. These functions are better than strncpy() and strncat() as their limit value is the size of the buffer (unlike strncat()), and the destination string will always be '\0'-terminated (unlike strncpy()). I have most of the changes written for an earlier version (rsync includes a version of 1.10.2 at present) that I could adapt for 1.14. ..wayne.. __ POPT Library http://rpm5.org Developer Communication List popt-devel@rpm5.org
Re: Allow equal after a short option
On Sat, Mar 08, 2008 at 12:10:52PM -0500, Jeff Johnson wrote: > Test "test1 -2 foo" failed with: "arg1: 0 arg2: rest: foo" != "arg1: > 0 arg2: foo" I'm not seeing that error with the CVS version. I do note that my prior patch to fix the longArg pointer (e.g. "./test1 -2foo=bar") isn't there, but even commenting out that fix doesn't get test 9 to fail for me. I did see failures for tests 19-23 due to the --echo-args macro now outputting an extra "--" at the start. The attached patch fixes this, and also adds two new tests to check that the changed equal-handling works right. ..wayne.. popt-tests.sh Description: Bourne shell script
Re: Enable more compiler warnings in gcc and fix the complaints
On Mar 8, 2008, at 12:10 PM, Wayne Davison wrote: The attached patch turns on more extensive warnings in gcc (-W) and then fixes a bunch of unused-arg warnings and one signed/unsigned comparison warning. Non-gcc compilers should be unaffected. This should help to find problems in the future, and will allow other packages (such as rsync) to compile the popt code with -W w/o compiler warnings showing up. ..wayne.. I'm not really sure that adding __attribute__((__unused__)) helps anything. All of the changes were already marked with splint /[EMAIL PROTECTED]@*/ which are regularly verified with "make lint". OTOH the patch hurts nothing but compiler portability complexity. Added, thanks for the patch. 73 de Jeff __ POPT Library http://rpm5.org Developer Communication List popt-devel@rpm5.org
Re: Allow equal after a short option
On Mar 8, 2008, at 12:02 PM, Wayne Davison wrote: I think it would be nice to allow an equal to separate a short option letter from its abutting argument. e.g. these commands using the test1 executable would all work the same: ./test1 -2 foo ./test1 -2=foo ./test1 -2foo ./test1 --arg2 foo ./test1 --arg2=foo Since this has been a syntax error in released versions of popt, this should not cause a compatibility problem. This fix requires my prior patch to make sure that short-option parsing doesn't have longArg set. I think the patch is a reasonable extension, but there's more to do: [EMAIL PROTECTED] popt]$ make check ... make check-TESTS make[2]: Entering directory `/X/src/popt' Running tests in /X/src/popt Running test test1 - 1. Running test test1 - 2. Running test test1 - 3. Running test test1 - 4. Running test test1 - 5. Running test test1 - 6. Running test test1 - 7. Running test test1 - 8. Running test test1 - 9. Test "test1 -2 foo" failed with: "arg1: 0 arg2: rest: foo" != "arg1: 0 arg2: foo" Would you mind looking at the problem? Thanks ... 73 de Jeff __ POPT Library http://rpm5.org Developer Communication List popt-devel@rpm5.org
Enable more compiler warnings in gcc and fix the complaints
The attached patch turns on more extensive warnings in gcc (-W) and then
fixes a bunch of unused-arg warnings and one signed/unsigned comparison
warning. Non-gcc compilers should be unaffected.
This should help to find problems in the future, and will allow other
packages (such as rsync) to compile the popt code with -W w/o compiler
warnings showing up.
..wayne..
--- popt-1.14/configure 2008-02-16 16:40:56 -0800
+++ configure 2008-03-08 08:48:08 -0800
@@ -19512,7 +19512,7 @@ LIBTOOL='$(SHELL) $(top_builddir)/libtoo
if test "X$CC" = Xgcc; then
-CFLAGS="-Wall $CFLAGS"
+CFLAGS="-Wall -W $CFLAGS"
fi
if test $ac_cv_c_compiler_gnu = yes; then
--- popt-1.14/configure.ac 2008-02-16 16:35:13 -0800
+++ configure.ac2008-03-08 08:47:04 -0800
@@ -21,7 +21,7 @@ AC_PROG_INSTALL
AC_PROG_LIBTOOL
if test "X$CC" = Xgcc; then
-CFLAGS="-Wall $CFLAGS"
+CFLAGS="-Wall -W $CFLAGS"
fi
AC_GCC_TRADITIONAL
--- popt-1.14/popt.c2008-03-08 08:20:20 -0800
+++ popt.c 2008-03-08 08:45:02 -0800
@@ -644,7 +644,7 @@ static void poptStripArg(/[EMAIL PROTECTED]@*/ p
/[EMAIL PROTECTED]@*/
}
-int poptSaveString(const char *** argvp, /[EMAIL PROTECTED]@*/ unsigned int
argInfo,
+int poptSaveString(const char *** argvp, /[EMAIL PROTECTED]@*/ UNUSED(unsigned
int argInfo),
const char * val)
{
poptArgv argv;
@@ -1218,7 +1218,7 @@ poptContext poptFreeContext(poptContext
}
int poptAddAlias(poptContext con, struct poptAlias alias,
- /[EMAIL PROTECTED]@*/ int flags)
+ /[EMAIL PROTECTED]@*/ UNUSED(int flags))
{
struct poptItem_s item_buf;
poptItem item = &item_buf;
--- popt-1.14/poptconfig.c 2008-02-16 16:35:14 -0800
+++ poptconfig.c2008-03-08 08:46:29 -0800
@@ -170,7 +170,7 @@ int poptReadConfigFile(poptContext con,
return 0;
}
-int poptReadDefaultConfig(poptContext con, /[EMAIL PROTECTED]@*/ int useEnv)
+int poptReadDefaultConfig(poptContext con, /[EMAIL PROTECTED]@*/ UNUSED(int
useEnv))
{
static const char _popt_sysconfdir[] = POPT_SYSCONFDIR "/popt";
static const char _popt_etc[] = "/etc/popt";
@@ -192,7 +192,7 @@ int poptReadDefaultConfig(poptContext co
glob_t g;
/[EMAIL PROTECTED] -nullpass -type @*/ /* FIX: annotations for glob/globfree */
if (!glob("/etc/popt.d/*", 0, NULL, &g)) {
-int i;
+ unsigned i;
for (i=0; i
Allow equal after a short option
I think it would be nice to allow an equal to separate a short option letter from its abutting argument. e.g. these commands using the test1 executable would all work the same: ./test1 -2 foo ./test1 -2=foo ./test1 -2foo ./test1 --arg2 foo ./test1 --arg2=foo Since this has been a syntax error in released versions of popt, this should not cause a compatibility problem. This fix requires my prior patch to make sure that short-option parsing doesn't have longArg set. ..wayne.. --- popt-1.14/popt.c2008-03-08 08:20:20 -0800 +++ popt.c 2008-03-08 08:45:02 -0800 @@ -937,7 +937,7 @@ int poptGetNextOpt(poptContext con) origOptString++; if (*origOptString != '\0') - con->os->nextCharArg = origOptString; + con->os->nextCharArg = origOptString + (*origOptString == '='); } if (opt == NULL) return POPT_ERROR_BADOPT; /* XXX can't happen */
Bug with equal in abutting short option
Popt has a bug where a short option with an abutting arg is parsed
incorrectly if there is an equal sign in the arg. This can be seen
using the "test1" executable:
% ./test1 -2foo-bar
arg1: 0 arg2: foo-bar
% ./test1 -2foo=bar
test1: bad argument -2foo=bar: invalid numeric value
% ./test1 -21=bar
test1: bad argument -21=bar: unknown option
In the failure cases, the code takes "bar" as the arg to the -2 option,
and then tries to continue parsing the characters that follow the -2
(thus the numeric error for the 'f' option in the first failure, and the
unknown option error for the '=' in the second).
The following patch for 1.14 resets the longArg variable if there was
not a single-dash long-named arg found for an option with a single dash:
--- popt.c.orig 2008-02-16 16:52:18 -0800
+++ popt.c 2008-03-08 00:39:17 -0800
@@ -901,6 +901,7 @@ int poptGetNextOpt(poptContext con)
if (!opt) {
con->os->nextCharArg = origOptString + 1;
+ longArg = NULL;
} else {
if (con->os == con->optionStack && F_ISSET(opt, STRIP))
{
This fixes the problem, and had no adverse affects on any of the various
ways of using -2 and --arg2 (and also -arg2 if POPT_ARGFLAG_ONEDASH is
enabled for that option).
..wayne..
__
POPT Library http://rpm5.org
Developer Communication List popt-devel@rpm5.org
