Bug report for APR [2023/12/03]
+---+ | Bugzilla Bug ID | | +-+ | | Status: UNC=Unconfirmed NEW=New ASS=Assigned| | | OPN=ReopenedVER=Verified(Skipped Closed/Resolved) | | | +-+ | | | Severity: BLK=Blocker CRI=Critical REG=Regression MAJ=Major | | | | MIN=Minor NOR=NormalENH=Enhancement TRV=Trivial | | | | +-+ | | | | Date Posted | | | | | +--+ | | | | | Description | | | | | | | |16056|Inf|Enh|2003-01-14|Shared memory & mutex ownership not correctly esta| |20382|New|Nor|2003-05-31|Poor performance on W2000 AS | |28453|New|Enh|2004-04-18|apr_uri should parse relative to a base URI | |33188|Inf|Nor|2005-01-21|libtool linking failure on Suse | |33490|Inf|Nor|2005-02-10|APR does not compile with Borland C++ | |38410|New|Nor|2006-01-27|apr/win32 misinterpreted the meaning of WAIT_ABAND| |39289|New|Enh|2006-04-12|test suite additions for trylock functions| |39853|Inf|Nor|2006-06-21|apr_strtoi64 does not build on WinCE due to lack o| |39895|New|Enh|2006-06-23|apr_os_strerror on WinCE needs to xlate unicode->u| |39896|New|Enh|2006-06-23|Output test status to OutputDebugString in additio| |40020|New|Enh|2006-07-11|Add support for apr_uint8_t and apr_int8_t types | |40193|Inf|Nor|2006-08-06|Patches to support different compiler than EMX on | |40622|New|Enh|2006-09-27|enhance apr temp files on NT to be more secure| |40758|Ver|Maj|2006-10-15|WIN64, apr_vformatter(..) cannot handle 64bit poin| |40939|New|Enh|2006-11-09|pool minimal allocation size should be configurabl| |41192|Inf|Trv|2006-12-17|Add the expat libtool file to the LT_LDFLAGS varia| |41254|New|Enh|2006-12-28|apr_queue_t enhancements | |41351|Inf|Enh|2007-01-11|Tivoli LDAP SDK support in aprutil| |41352|Inf|Min|2007-01-11|openldap and per-connection client certificates in| |41916|Inf|Nor|2007-03-21|MinGW cross-compile support for Linux | |42365|New|Enh|2007-05-09|Suppress console for apr_proc_create() created pro| |42682|New|Maj|2007-06-17|Apache child terminates with signal 11 when using | |42728|New|Nor|2007-06-23|mod_ssl thread detaching not releasing handles| |42848|New|Enh|2007-07-10|add IP TOS support to apr_socket_opt_set()| |43035|New|Enh|2007-08-04|Add ability to wrap ssl around pre-existing socket| |43066|New|Nor|2007-08-08|get_password on Windows is kludgy | |43152|Inf|Nor|2007-08-16|apr_socket_opt_get doesn't work with APR_SO_SNDBUF| |43172|Ass|Nor|2007-08-20|apr-util don't want to find mozldap-6.x | |43217|New|Min|2007-08-26|All-ones IPv6 subnet mask not accepted| |43244|New|Enh|2007-08-29|apr_socket_t missing dup, dup2 and setaside | |43302|New|Nor|2007-09-04|apr_bucket_socket doesn't work with non-connected | |43309|New|Enh|2007-09-05|add apr_socket_sendtov support| |43375|New|Nor|2007-09-13|Pool integrity check fails for apr threads| |43499|New|Nor|2007-09-27|apr_global_mutex_create returns error "Access deni| |43507|New|Enh|2007-09-28|configure flag for SHELL_PATH | |43508|New|Enh|2007-09-28|Please be available whether atomics use thread mut| |43793|New|Enh|2007-11-04|builtin atomics on Windows| |44127|New|Enh|2007-12-21|File Extended Attributes Support | |44128|New|Enh|2007-12-21|apr_os_file_put() does not register a cleanup hand| |44129|New|Enh|2007-12-21|apr_os_dir_put() does not allocate an entry buffer| |44186|New|Nor|2008-01-08|[PATCH] Add memcached 1.2.4 features to apr_memcac| |44230|New|Enh|2008-01-14|Add Ark Linux support to config.layout| |44432|New|Enh|2008-02-15|patch - proposal for application function hooks | |44550|Inf|Maj|2008-03-06|Solaris sendfilev() handling - EINTR appears to ha| |45251|New|Nor|2008-06-22|DBD MySQL driver doesn't support multiple resultse| |45291|New|Nor|2008-06-26|apr_thread_t is leaking | |45298|New|Maj|2008-06-27|apr_os_thread_get() differs between windows and un| |45407|Opn|Nor|2008-07-16|auto reconnect in apr_dbd_mysql disturb normal wor| |45494|Opn|Nor|2008-07-28|testsockets fails on Solaris when IPv6 interfaces | |45496|New|Enh|2008-07-29|[patch] adding directory matching [dir/**/conf.d/*| |45700|New|Nor|2008-08-27|incorrect sort order of apr_strnatcmp with non-ASC|
Re: getopt error message with a bare "-"
Den lör 2 dec. 2023 kl 11:08 skrev Graham Leggett : > On 28 Nov 2023, at 07:46, Daniel Sahlberg > wrote: > > Subversion is using APR's apr_getopt_long for command line processing. > > A user tried to supply "-" for the filename with the intention of using > stdin (Subversion has support for this in our code). However the user got > the following error message (with the first line coming from > apr_getopt_long and the second from Subversion's code): > > $ ./svnmucc put - file:///url/to/repository > svnmucc: invalid option: > Type 'svnmucc --help' for usage. > > We found the solution, to use "--" to disable option processing, so no > discussion around this is needed. > > In the thread, someone mentioned that it would be useful if the error > message could be improved so I looked into the APR code and have a > suggestion. > > The following code is responsible for the error message. In other places, > the error message contains the offending (long) option (third argument). > However since *p is always \0, checked just above, the error message will > just say "invalid option". I'm suggesting to change this to the fixed > string "-" instead. > [[[ > --- getopt.orig 2023-11-28 08:22:16.690508444 +0100 > +++ getopt.c2023-11-28 08:22:50.270297468 +0100 > @@ -272,7 +272,7 @@ > } > else > if (*p == '\0')/* Bare "-" is illegal > */ > -return serr(os, "invalid option", p, APR_BADCH); > +return serr(os, "invalid option", "-", APR_BADCH); > } > } > ]]] > > With this change, the error message will instead be: > $ ./svnmucc put - file:///url/to/repository > svnmucc: invalid option: - > Type 'svnmucc --help' for usage. > > with the "-" indicating which part of the command line is invalid. > > Original e-mail from our users@ list: > https://lists.apache.org/thread/mbsx7rxpx07sh6sfs6xhbnvgo2wbfl0k > > Follow-up e-mail in our dev@ list: > https://lists.apache.org/thread/l7x22yp6kb71qlv3rn8tfmcrc5hk73r4 > > > Waves hi :) > Oh, hi! :) (Graham is the user I mentioned above, didn't know you were here ... ) > > +1 on the above message fix at the very least. I’ll commit it if there are > no objections. > > Another solution would be to add a switch to getopt.c that allows for - to > be counted not as a "start of short option string" but as a separate > argument, possibly hidden behind a switch in the apr_getopt_t struct. For > our use, that would be even better, but I assume it would require more code > (also on our side to remain backwards compatible with older APR versions). > > > Looks like apr_getopt_t is the right place for a flag. It seems a lot more > intuitive to have “-“ recognised as an argument rather than an invalid > option. > That's exactly how I saw it as well. Not sure if we (Subversion) would even make use of that option for a long time, since we need to be reasonably sure an uptodate version of APR is available before we release. > > Regards, > Graham > — > Kind regards Daniel >
Re: getopt error message with a bare "-"
On 28 Nov 2023, at 07:46, Daniel Sahlberg wrote: > Subversion is using APR's apr_getopt_long for command line processing. > > A user tried to supply "-" for the filename with the intention of using stdin > (Subversion has support for this in our code). However the user got the > following error message (with the first line coming from apr_getopt_long and > the second from Subversion's code): > > $ ./svnmucc put - file:///url/to/repository > svnmucc: invalid option: > Type 'svnmucc --help' for usage. > > We found the solution, to use "--" to disable option processing, so no > discussion around this is needed. > > In the thread, someone mentioned that it would be useful if the error message > could be improved so I looked into the APR code and have a suggestion. > > The following code is responsible for the error message. In other places, the > error message contains the offending (long) option (third argument). However > since *p is always \0, checked just above, the error message will just say > "invalid option". I'm suggesting to change this to the fixed string "-" > instead. > [[[ > --- getopt.orig 2023-11-28 08:22:16.690508444 +0100 > +++ getopt.c2023-11-28 08:22:50.270297468 +0100 > @@ -272,7 +272,7 @@ > } > else > if (*p == '\0')/* Bare "-" is illegal */ > -return serr(os, "invalid option", p, APR_BADCH); > +return serr(os, "invalid option", "-", APR_BADCH); > } > } > ]]] > > With this change, the error message will instead be: > $ ./svnmucc put - file:///url/to/repository > svnmucc: invalid option: - > Type 'svnmucc --help' for usage. > > with the "-" indicating which part of the command line is invalid. > > Original e-mail from our users@ list: > https://lists.apache.org/thread/mbsx7rxpx07sh6sfs6xhbnvgo2wbfl0k > > Follow-up e-mail in our dev@ list: > https://lists.apache.org/thread/l7x22yp6kb71qlv3rn8tfmcrc5hk73r4 Waves hi :) +1 on the above message fix at the very least. I’ll commit it if there are no objections. > Another solution would be to add a switch to getopt.c that allows for - to be > counted not as a "start of short option string" but as a separate argument, > possibly hidden behind a switch in the apr_getopt_t struct. For our use, that > would be even better, but I assume it would require more code (also on our > side to remain backwards compatible with older APR versions). Looks like apr_getopt_t is the right place for a flag. It seems a lot more intuitive to have “-“ recognised as an argument rather than an invalid option. Regards, Graham —
