Re: Disallowing args
On Thu, Jun 17, 2010 at 9:21 AM, Jeff Johnson wrote: > Meanwhile in the future, if you could send a patch against some POPT > release that I can find > Sorry about that. I sent you a patch for the rsync popt by mistake. Here's one I just made against 1.16 that now includes a couple tests for the arg rejection. http://opencoder.net/arg-error.patch ..wayne..
Re: POPT's API has designed in memory leaks. What to do?
On Mon, Jun 7, 2010 at 9:40 AM, Jeff Johnson wrote: > The added tyrrany of forcing every application that currently has > -lpopt > to change to > -ljdod > will be rate-determining to adoption (and glacially/tectonically slow imho) > For me, if popt 2 is not compatible with popt 1 then I would rather have to make a conscious choice to upgrade my code (testing it for compatibility), and having to change the libray name as a part of that is pretty minor. Having a possibility for my program to be run-time linked with a library that is not compatible with what it expects would be very bad, imo. ..wayne..
Re: POPT's API has designed in memory leaks. What to do?
On Mon, Jun 7, 2010 at 9:14 AM, Jeff Johnson wrote: > And without some deterministic way to tell whether its a POPT 1.x <-> 2.x > table being fed to the POPT API/ABI, well, only a deliberately > "incompatible" POPT 2.0 data structure with some deterministic > version identifier is possible. > One way to deal with this is to change the library name to "popt2". That way, a popt (1.x) using program would never get run with a popt2 library. ..wayne..
Re: Disallowing args
On Sat, Jun 5, 2010 at 8:56 AM, Jeff Johnson wrote: > but >--foo bar > returns bar in the argument list? > Yes. The user may well have wanted it to be in the arg list. There's no way for the program to know that the user didn't just toss some options in the middle of some args (which I do all the time these days, like starting my rsync source/dest args, and then tossing in a --remove-source-files, --backup, or what-not at wherever I am in the list), so I wouldn't want to see an error for something like this: rsync -aiv --del file --remove-source-args some/dir host:/dest/dir ... just because --del doesn't take an arg. The other way to "fix" the error is to morph "--foo=bar" behavior to be > identical to "--foo bar", i.e. an extra argument failure. > I don't see how that would work for something like rsync that takes any number of command-line args outside the options. Anything you want to see in POPT 2.0? I'm collecting features ... A couple ideas off the top of my head: - An incrementing option -- repeated use adds 1 to the variable instead of setting it to the same value. - Multiple long names separated by "|" in the long-name string (though that could really just be defined as an alias, it might be nice to auto-gen the alias). ..wayne..
Disallowing args
Here's something that was recently fixed for the popt that is included
with rsync: rejecting an arg to an option that doesn't take an arg.
Attached is a patch. A new error code, POPT_ERROR_UNWANTEDARG, was
created to make the error message nice. This handles both -l=value
and --long-arg=value where neither one is supposed to take a value.
..wayne..
--- popt.c
+++ popt.c
@@ -860,20 +860,21 @@ int poptGetNextOpt(poptContext con)
origOptString++;
if (*origOptString != '\0')
- con->os->nextCharArg = origOptString + (*origOptString == '=');
+ con->os->nextCharArg = origOptString;
}
/*...@=branchstate@*/
if (opt == NULL) return POPT_ERROR_BADOPT; /* XXX can't happen */
- if (opt->arg && (opt->argInfo & POPT_ARG_MASK) == POPT_ARG_NONE) {
- if (poptSaveInt((int *)opt->arg, opt->argInfo, 1L))
- return POPT_ERROR_BADOPERATION;
- } else if ((opt->argInfo & POPT_ARG_MASK) == POPT_ARG_VAL) {
+ if ((opt->argInfo & POPT_ARG_MASK) == POPT_ARG_NONE
+ || (opt->argInfo & POPT_ARG_MASK) == POPT_ARG_VAL) {
+ if (longArg || (con->os->nextCharArg && con->os->nextCharArg[0] == '='))
+ return POPT_ERROR_UNWANTEDARG;
if (opt->arg) {
- if (poptSaveInt((int *)opt->arg, opt->argInfo, (long)opt->val))
+ long val = (opt->argInfo & POPT_ARG_MASK) == POPT_ARG_VAL ? opt->val : 1;
+ if (poptSaveInt((int *)opt->arg, opt->argInfo, val))
return POPT_ERROR_BADOPERATION;
}
- } else if ((opt->argInfo & POPT_ARG_MASK) != POPT_ARG_NONE) {
+ } else {
con->os->nextArg = _free(con->os->nextArg);
/*...@-usedef@*/ /* FIX: W2DO? */
if (longArg) {
@@ -881,7 +882,7 @@ int poptGetNextOpt(poptContext con)
longArg = expandNextArg(con, longArg);
con->os->nextArg = longArg;
} else if (con->os->nextCharArg) {
- longArg = expandNextArg(con, con->os->nextCharArg);
+ longArg = expandNextArg(con, con->os->nextCharArg + (con->os->nextCharArg[0] == '='));
con->os->nextArg = longArg;
con->os->nextCharArg = NULL;
} else {
@@ -1202,6 +1203,8 @@ const char * poptStrerror(const int error)
switch (error) {
case POPT_ERROR_NOARG:
return POPT_("missing argument");
+ case POPT_ERROR_UNWANTEDARG:
+ return POPT_("option does not take an argument");
case POPT_ERROR_BADOPT:
return POPT_("unknown option");
case POPT_ERROR_BADOPERATION:
diff --git a/popt/popt.h b/popt/popt.h
index 4f85d9e..8d85f73 100644
--- a/popt/popt.h
+++ b/popt/popt.h
@@ -82,6 +82,7 @@
/*...@{*/
#define POPT_ERROR_NOARG -10 /*!< missing argument */
#define POPT_ERROR_BADOPT -11 /*!< unknown option */
+#define POPT_ERROR_UNWANTEDARG -12 /*!< option does not take an argument */
#define POPT_ERROR_OPTSTOODEEP -13 /*!< aliases nested too deeply */
#define POPT_ERROR_BADQUOTE -15 /*!< error in paramter quoting */
#define POPT_ERROR_ERRNO -16 /*!< errno set, use strerror(errno) */
A little more stpcpy() tweaking + a "..." bonus
I made a couple more places use stpcpy() in the code, optimizing away a
couple strlen() calls in the process. I also tweaked the help code that
was abbreviating a long value using "..." to allow a string to fit if it
can, rather than always reserving space for the "..." chars. I.e. if we
had room for 4 word chars, instead of outputting "w...", it would allow
a full "word" to be output, since it fits.
..wayne..
--- popthelp.c 10 Mar 2008 08:13:09 - 1.86
+++ popthelp.c 15 Mar 2008 05:09:45 -
@@ -271,12 +271,12 @@ singleOptionDefaultValue(size_t lineLeng
if (s == NULL)
le = stpcpy(le, "null");
else {
- size_t slen = 4*lineLength - (le - l) - sizeof("\"...\")");
+ size_t limit = 4*lineLength - (le - l) - sizeof("\"\")");
+ size_t slen;
*le++ = '"';
- strncpy(le, s, slen); le[slen] = '\0'; le += strlen(le);
- if (slen < strlen(s)) {
- strcpy(le, "..."); le += strlen(le);
- }
+ strncpy(le, s, limit); le[limit] = '\0'; le += (slen = strlen(le));
+ if (slen == limit && s[limit])
+ le[-1] = le[-2] = le[-3] = '.';
*le++ = '"';
}
} break;
@@ -363,7 +363,6 @@ static void singleOptionHelp(FILE * fp,
strlen(defs) + sizeof(" "));
if (t) {
char * te = t;
- *te = '\0';
if (help)
te = stpcpy(te, help);
*te++ = ' ';
@@ -417,24 +416,22 @@ static void singleOptionHelp(FILE * fp,
case POPT_ARG_DOUBLE:
case POPT_ARG_STRING:
*le++ = (opt->longName != NULL ? '=' : ' ');
- strcpy(le, argDescrip); le += strlen(le);
+ le = stpcpy(le, argDescrip);
break;
default:
break;
}
} else {
- size_t lelen;
+ char *leo;
/* XXX argDescrip[0] determines "--foo=bar" or "--foo bar". */
if (!strchr(" =(", argDescrip[0]))
*le++ = ((poptArgType(opt) == POPT_ARG_MAINCALL) ? ' ' :
(poptArgType(opt) == POPT_ARG_ARGV) ? ' ' : '=');
- strcpy(le, argDescrip);
- lelen = strlen(le);
- le += lelen;
+ le = stpcpy(leo = le, argDescrip);
/* Adjust for (possible) wide characters. */
- displaypad = (int)(lelen - stringDisplayWidth(argDescrip));
+ displaypad = (int)((le - leo) - stringDisplayWidth(argDescrip));
}
if (F_ISSET(opt, OPTIONAL))
*le++ = ']';
Re: Question about the xmalloc() et al functions
On Sun, Mar 09, 2008 at 07:00:52PM -0700, Wayne Davison wrote:
> +} else if (prtshort) {
> + left[0] = '-';
> + left[1] = opt->shortName;
> +} else if (prtlong) {
Oops, there should have been a left[2] = '\0' in there.
..wayne..
__
POPT Library http://rpm5.org
Developer Communication List popt-devel@rpm5.org
Re: Question about the xmalloc() et al functions
On Sun, Mar 09, 2008 at 08:27:25PM -0400, Jeff Johnson wrote:
> Using xmalloc just opens up a can-of-worms while lusers fuss about
> non-gcc compiler extension portability.
Aha, I had failed to notice that the "? :" bit was a gcc extension.
What I don't like is that normal users get some memory functions that
are fatal if they use gcc, while all others get non-fatal functions.
Perhaps the default should be for all normal compiles to get the non-
fatal functions, and then anyone that wanted to use the current gcc
defines could set something extra (such as -DFATAL_MEM).
One other reason I care about this is that I think the various bits of
code that are doing a strdup()-like action should be using a strdup()-
like call, not each one having their own set of strlen/malloc/strcpy
calls. It's clearer and a bit safer.
I'm attaching a patch that adds the extra FATAL_MEM requirement, and
then uses xstrdup() in a few places (which would mean that strdup() is
being used for normal popt users).
I was also working on changing some code to use your newly-added
stpcpy() function, so I included that work too. Some of the changes
are optimizations to avoid a strlen() call (when we can easily compute
the length via subtraction) and a fix for a potential memory leak if
realloc() returns NULL.
..wayne..
--- system.h9 Mar 2008 20:24:45 - 1.14
+++ system.h10 Mar 2008 01:40:23 -
@@ -78,7 +78,7 @@ static inline char * stpcpy (char *dest,
#endif
/* Memory allocation via macro defs to get meaningful locations from mtrace()
*/
-#if defined(HAVE_MCHECK_H) && defined(__GNUC__)
+#if defined(FATAL_MEM) && defined(HAVE_MCHECK_H) && defined(__GNUC__)
#definevmefail() (fprintf(stderr, "virtual memory
exhausted.\n"), exit(EXIT_FAILURE), NULL)
#definexmalloc(_size) (malloc(_size) ? : vmefail())
#definexcalloc(_nmemb, _size) (calloc((_nmemb), (_size)) ? :
vmefail())
--- findme.c9 Mar 2008 20:24:45 - 1.19
+++ findme.c10 Mar 2008 01:40:21 -
@@ -23,11 +23,10 @@ const char * POPT_findProgramPath(const
if (path == NULL) return NULL;
-start = pathbuf = malloc(strlen(path) + 1);
+start = pathbuf = xstrdup(path);
if (pathbuf == NULL) goto exit;
buf = malloc(strlen(path) + strlen(argv0) + sizeof("/"));
if (buf == NULL) goto exit;
-strcpy(pathbuf, path);
chptr = NULL;
do {
--- popt.c 9 Mar 2008 23:10:05 - 1.120
+++ popt.c 10 Mar 2008 01:40:22 -
@@ -196,10 +196,8 @@ poptContext poptGetContext(const char *
if (getenv("POSIXLY_CORRECT") || getenv("POSIX_ME_HARDER"))
con->flags |= POPT_CONTEXT_POSIXMEHARDER;
-if (name) {
- char * t = malloc(strlen(name) + 1);
- if (t) con->appName = strcpy(t, name);
-}
+if (name)
+ con->appName = xstrdup(name);
invokeCallbacksPRE(con, con->options);
@@ -593,7 +591,7 @@ expandNextArg(/[EMAIL PROTECTED]@*/ poptContext
/[EMAIL PROTECTED] con @*/
{
const char * a = NULL;
-size_t alen;
+size_t alen, pos;
char *t, *te;
size_t tn = strlen(s) + 1;
char c;
@@ -619,10 +617,9 @@ expandNextArg(/[EMAIL PROTECTED]@*/ poptContext
alen = strlen(a);
tn += alen;
- *te = '\0';
+ pos = te - t;
t = realloc(t, tn);
- te = t + strlen(t);
- strncpy(te, a, alen); te += alen;
+ te = stpcpy(t + pos, a);
continue;
/[EMAIL PROTECTED]@*/ /[EMAIL PROTECTED]@*/ break;
default:
@@ -630,8 +627,13 @@ expandNextArg(/[EMAIL PROTECTED]@*/ poptContext
}
*te++ = c;
}
-*te = '\0';
-t = realloc(t, strlen(t) + 1); /* XXX memory leak, hard to plug */
+*te++ = '\0';
+if (t + tn > te) {
+ char *ot = t;
+ t = realloc(t, te - t);
+ if (!t)
+ free(ot);
+}
return t;
}
--- poptconfig.c9 Mar 2008 20:24:45 - 1.37
+++ poptconfig.c10 Mar 2008 01:40:22 -
@@ -224,8 +224,7 @@ int poptReadDefaultConfig(poptContext co
if ((home = getenv("HOME"))) {
fn = malloc(strlen(home) + 20);
if (fn != NULL) {
- strcpy(fn, home);
- strcat(fn, "/.popt");
+ (void)stpcpy(stpcpy(fn, home), "/.popt");
rc = poptReadConfigFile(con, fn);
free(fn);
} else
--- popthelp.c 8 Mar 2008 17:26:30 - 1.85
+++ popthelp.c 10 Mar 2008 01:40:23 -
@@ -237,7 +237,7 @@ singleOptionDefaultValue(size_t lineLeng
if (le == NULL) return NULL; /* XXX can't happen */
*le = '\0';
*le++ = '(';
-strcpy(le, defstr);le += strlen(le);
+le = stpcpy(le, defstr);
*le++ = ':';
*le++ = ' ';
if (opt->arg) { /* XXX programmer error */
@@ -269,7 +269,7 @@ singleOptionDefaultValue(size_t lineLeng
case POPT_ARG_STRING:
{ const char * s = arg.argv[0];
if (s == NULL) {
- s
Re: Allow equal after a short option
On Sun, Mar 09, 2008 at 07:10:21PM -0400, Jeff Johnson wrote: > Your original patch for -c=foo is now checked in. Cool! You should also be able to uncomment the extra tests now. ..wayne.. __ POPT Library http://rpm5.org Developer Communication List popt-devel@rpm5.org
Re: Question about the xmalloc() et al functions
On Sun, Mar 09, 2008 at 07:26:31PM -0400, Jeff Johnson wrote: > There are some subtleties however with xstrdup. [... mtrace related > issues ...] Right, that's why I preserved the HAVE_MCHECK_H version using its own malloc() call (it's unchanged from your version) and just made the non- HAVE_MCHECK_H version add failure checking. All the other functions are the same for both (with exit-on-failure handling). ..wayne.. __ POPT Library http://rpm5.org Developer Communication List popt-devel@rpm5.org
Question about the xmalloc() et al functions
I'm curious why the xmalloc() function (and its brethren) exits with a fatal error on only some systems and not on all systems? I would think that the code should use the exiting functions in all cases, and just provide differing versions of xstrdup() so that the HAVE_MCHECK_H version does a malloc() (rather than strdup() doing it). If I'm not off track, you can apply the attached patch to implement this. ..wayne.. --- system.h9 Mar 2008 20:24:45 - 1.14 +++ system.h9 Mar 2008 23:01:40 - @@ -77,19 +77,17 @@ static inline char * stpcpy (char *dest, } #endif -/* Memory allocation via macro defs to get meaningful locations from mtrace() */ -#if defined(HAVE_MCHECK_H) && defined(__GNUC__) +/* Error-checking versions of common memory-allocation functions. */ #definevmefail() (fprintf(stderr, "virtual memory exhausted.\n"), exit(EXIT_FAILURE), NULL) #definexmalloc(_size) (malloc(_size) ? : vmefail()) #definexcalloc(_nmemb, _size) (calloc((_nmemb), (_size)) ? : vmefail()) #definexrealloc(_ptr, _size) (realloc((_ptr), (_size)) ? : vmefail()) +/* Avoid strdup() to get meaningful locations from mtrace() */ +#if defined(HAVE_MCHECK_H) && defined(__GNUC__) #define xstrdup(_str) (strcpy((malloc(strlen(_str)+1) ? : vmefail()), (_str))) #else -#definexmalloc(_size) malloc(_size) -#definexcalloc(_nmemb, _size) calloc((_nmemb), (_size)) -#definexrealloc(_ptr, _size) realloc((_ptr), (_size)) -#definexstrdup(_str) strdup(_str) -#endif /* defined(HAVE_MCHECK_H) && defined(__GNUC__) */ +#define xstrdup(_str) (strdup(_str) ? : vmefail()) +#endif #if defined(HAVE___SECURE_GETENV) && !defined(__LCLINT__) #definegetenv(_s) __secure_getenv(_s)
Re: Allow equal after a short option
On Sat, Mar 08, 2008 at 12:10:52PM -0500, Jeff Johnson wrote:
> Running test test1 - 9.
> Test "test1 -2 foo" failed with: "arg1: 0 arg2: rest: foo" != "arg1:
> 0 arg2: foo"
I can get that failure if the line I added does not replace the prior
assignment (which makes it affect the case where *origOptString == '\0'
as well as the desired case where it is not '\0').
That's the only explanation I can come up with for why the code would
fail. I have attached a patch that codes up the increment in a slightly
different way, but I don't see how this change is any different on the
code that follows than what was there before. (Still, I might have
missed something...)
..wayne..
--- popt.c 9 Mar 2008 20:24:45 - 1.119
+++ popt.c 9 Mar 2008 22:15:08 -
@@ -931,11 +931,11 @@ int poptGetNextOpt(poptContext con)
shorty = 1;
origOptString++;
- if (*origOptString != '\0')
+ if (*origOptString != '\0') {
+ if (*origOptString == '=')
+ origOptString++;
con->os->nextCharArg = origOptString;
-#ifdef NOTYET /* XXX causes test 9 failure. */
- con->os->nextCharArg = origOptString + (*origOptString == '=');
-#endif
+ }
}
if (opt == NULL) return POPT_ERROR_BADOPT; /* XXX can't happen */
echo incompatibility (was Re: Allow equal after a short option)
On Sun, Mar 09, 2008 at 10:51:42AM -0400, Jeff Johnson wrote: > [EMAIL PROTECTED] popt]$ /bin/echo --foo --bar > --bar > [EMAIL PROTECTED] popt]$ /bin/echo -- --foo --bar > --foo --bar OK, Let's hope that your echo will not drop anything if the first arg passed to it is a non-option (such as "args:"). If so, the attached patch should work for everyone (it does for me). ..wayne.. --- test-poptrc 16 Feb 2008 22:16:10 - 1.4 +++ test-poptrc 9 Mar 2008 22:15:08 - @@ -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 echo args: test1 alias -e --echo-args -test1 exec -a /bin/echo -- +test1 exec -a /bin/echo args: --- testit.sh 8 Mar 2008 23:11:56 - 1.24 +++ testit.sh 9 Mar 2008 22:15:08 - @@ -68,11 +68,11 @@ POSIXLY_CORRECT=1 ; export POSIXLY_CORRE run test1 "test1 - 17" "arg1: 1 arg2: (none) rest: foo --arg2 something" --arg1 foo --arg2 something unset POSIXLY_CORRECT run test1 "test1 - 18" "callback: c sampledata bar arg1: 1 arg2: (none)" --arg1 --cb bar -run test1 "test1 - 19" "" --echo-args -run test1 "test1 - 20" "--arg1" --echo-args --arg1 -run test1 "test1 - 21" "--arg2 something" -T something -e -run test1 "test1 - 22" "--arg2 something more args" -T something -a more args -run test1 "test1 - 23" "--echo-args -a" --echo-args -e -a +run test1 "test1 - 19" "args:" --echo-args +run test1 "test1 - 20" "args: --arg1" --echo-args --arg1 +run test1 "test1 - 21" "args: --arg2 something" -T something -e +run test1 "test1 - 22" "args: --arg2 something more args" -T something -a more args +run test1 "test1 - 23" "args: --echo-args -a" --echo-args -e -a run test1 "test1 - 24" "arg1: 0 arg2: (none) short: 1" -onedash run test1 "test1 - 25" "arg1: 0 arg2: (none) short: 1" --onedash run test1 "test1 - 26" "callback: c arg for cb2 foo arg1: 0 arg2: (none)" --cb2 foo
Re: Allow equal after a short option
On Sun, Mar 09, 2008 at 10:51:42AM -0400, Jeff Johnson wrote:
> Meanwhile, below is a rewrite of POPT_fprintf, essentially identical
> to the "man vsnprintf" example. See what you think ...
Looks good. I did some tweaking based on this code. The attached patch
changes what you checked-in to CVS. See if you agree with these points:
1. If the system has vasprintf(), just use that (avoiding issues 2 & 3).
2. Always allocate one more byte than we need, just in case the system's
vsprintf() has an off-by-one bug with the limit.
3. Moved the va_start() and va_end() calls back in the loop because I
believe that the systems that needed a va_copy() in the old code
won't like the reuse of "ap" without it.
Bonus question: I didn't think older systems supported the idiom of
realloc(NULL, len) doing a malloc(). I didn't check to see if that
idiom is already used elsewhere in the popt code, though, so I just
left that alone.
I tested both sides of the HAVE_VASPRINTF code and this seems good to
me.
..wayne..
--- configure.ac9 Mar 2008 20:24:45 - 1.39
+++ configure.ac9 Mar 2008 21:26:17 -
@@ -69,7 +69,7 @@ AM_CONDITIONAL(HAVE_LD_VERSION_SCRIPT, t
AC_CHECK_FUNC(setreuid, [], [
AC_CHECK_LIB(ucb, setreuid, [if echo $LIBS | grep -- -lucb >/dev/null
;then :; else LIBS="$LIBS -lc -lucb" USEUCB=y;fi])
])
-AC_CHECK_FUNCS(getuid geteuid iconv mtrace __secure_getenv setregid stpcpy
strerror)
+AC_CHECK_FUNCS(getuid geteuid iconv mtrace __secure_getenv setregid stpcpy
strerror vasprintf)
AM_GNU_GETTEXT([external])
--- poptint.c 9 Mar 2008 20:24:45 - 1.17
+++ poptint.c 9 Mar 2008 21:26:18 -
@@ -152,13 +152,17 @@ int
POPT_fprintf (FILE * stream, const char * format, ...)
{
char * b = NULL, * ob = NULL;
-size_t nb = (size_t)1;
int rc;
va_list ap;
+#ifndef HAVE_VASPRINTF
+size_t nb = (size_t)1;
-va_start(ap, format);
-while ((b = realloc(b, nb)) != NULL) {
+/* Note that we always allocate 1 byte more than we need, just in
+ * case the vsnprintf() has an off-by-one bug with the limit. */
+while ((b = realloc(b, nb+1)) != NULL) {
+ va_start(ap, format);
rc = vsnprintf(b, nb, format, ap);
+ va_end(ap);
if (rc > -1) { /* glibc 2.1 */
if ((size_t)rc < nb)
break;
@@ -167,8 +171,16 @@ POPT_fprintf (FILE * stream, const char
nb += (nb < (size_t)100 ? (size_t)100 : nb);
ob = b;
}
+
+#else /* HAVE_VASPRINTF */
+
+va_start(ap, format);
+if (vasprintf(&b, format, ap) < 0)
+ b = NULL;
va_end(ap);
+#endif
+
rc = 0;
if (b != NULL) {
#ifdef HAVE_ICONV
Re: Allow equal after a short option
On Sun, Mar 09, 2008 at 12:42:37AM -0500, Jeff Johnson wrote:
> /bin/echo on my system is unmodified from
> Fedora 9 coreutils-6.10-4.fc9.i386
Interesting. So, what do you get with a manual run?
/bin/echo --foo --bar
/bin/echo -- --foo --bar
I see all the option information output literally, including the --.
What do you get if you try a "make check" using that perl -e patch
instead of echo? Does it still succeed for you?
> I added the longArg = NULL, am seeing the same failure on test # 9.
Very weird. I don't see how my change could affect a short option's
separated arg. Attached is an even safer version of the change that
ensures that the only time it ever sets longArg to NULL is if the
longArg was set to oe + 1 upon finding an equal sign.
I also tried using valgrind in the test suite:
result=`valgrind $builddir/$prog $*`
and the test-run didn't turn up any errors.
..wayne..
--- popt.c 9 Mar 2008 06:01:05 - 1.118
+++ popt.c 9 Mar 2008 14:03:00 -
@@ -883,6 +883,8 @@ int poptGetNextOpt(poptContext con)
optStringLen = (size_t)(oe - optString);
if (*oe == '=')
longArg = oe + 1;
+ else
+ oe = NULL;
/* XXX aliases with arg substitution need "--alias=arg" */
if (handleAlias(con, optString, optStringLen, '\0', longArg)) {
@@ -895,13 +897,16 @@ int poptGetNextOpt(poptContext con)
opt = findOption(con->options, optString, optStringLen, '\0',
&cb, &cbData,
singleDash);
- if (!opt && !singleDash)
- return POPT_ERROR_BADOPT;
+ if (!opt) {
+ if (!singleDash)
+ return POPT_ERROR_BADOPT;
+ if (oe)
+ longArg = NULL;
+ }
}
if (!opt) {
con->os->nextCharArg = origOptString + 1;
- longArg = NULL;
} else {
if (con->os == con->optionStack && F_ISSET(opt, STRIP))
{
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))
{
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
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
