From: Dale Mellor <guile-qf1qmg@rdmp.org>
To: 40719@debbugs.gnu.org
Subject: bug#40719: [PATCH 0/4] GNU Mcron and the (ice-9 getopt-long) module
Date: Sun, 19 Apr 2020 18:47:22 +0100 [thread overview]
Message-ID: <c991d9c48eef80a68274ee28a0c1040b4eea4953.camel@rdmp.org> (raw)
/Mcron/ is a GNU package which runs unattended jobs in the operating
system at dynamically computed times; it is 99% Guile but currently
shrouded in a thin veneer of C code for historical reasons, which
have by now vanished.
The Guile /getopt-long/ module parses a command lineʼs arguments for
options and their values according to a provided grammar.
In the process of removing the thin veneer of C code from around the
/GNU Mcron/ package, I am running up against niggles in the
implementation of the /(ice-9 getopt-long)/ module. The intention
with /mcron/ has always been that a command-line argument be
provided which allows the user to request the display of the next
eight jobs to run, or allows the user to specify the number of such
jobs. Thus the intention was that command-lines like ‘mcron -s4
file’, ‘mcron -s 4 file’, and ‘mcron -s file’ would all work; alas,
the last one, actually the most important case, doesnʼt with the
current module, which issues a fatal exit on the grounds that ‘file’
fails to meet predicated requirements of the option for ‘-s’ that it
should represent a decimal number.
It is clear that /getopt-long/ can do better than this, especially
if the consumer of the module provides predicates on values which
options can take, e.g. value should be numerical. It can then
objectively decide that an argument should be taken to be a value,
an option itself, or a ‘loose’ argument.
There are other problems which can be cleared up with the enhanced
logic, as outlined in Point 2 below.
The following patches clear up the situation.
1) The first patch introduces some 28 new tests of the existing
/getopt-long/ module; these are non-controversial and the current
code passes all the tests, but they exercise more of the corner
cases and provide confidence that a new implementation does not
break existing behaviour.
2) The second patch inverts one test which I disagree with (see
Point 3, below), and introduces 18 more tests which represent
currently indeterminate and unsupported behaviour, some
nevertheless desired by /mcron/; all of these create either test
FAIL cases with the current code-base, or total panic-escape from
the calling application.
Some specific test failures:
1. A command-line like ‘foo --test=’ produces a /test/ result with
the empty string as value; I would expect /#t/ as the value
(which indicates that the option is there but has no given
value).
2. A command-line with a negative number always errors. According
to the in-line documentation negative numbers canʼt ever appear
loose on the command-line, but this seems like a case which
might be realistic in real life and there is no reason to
reject them.
3. A command like ‘foo -abc d’ in which /b/ takes a mandatory
argument and /c/ is an allowed option, errors out, but in my
opinion in this case /b/ should take “c” as its value and the
command-line as a whole is *not* erroneous. If /b/ takes an
optional value things are more tricky to deal with, but if
there is a predicate on the values which /b/ can take, then the
parser can make a clearer decision on taking /c/ as a value or
another option.
This might seem picky, but the problem is that command-lines
are supplied by (possibly hostile) end-users, *not* by the
/getopt-long/ module, and not by the application which consumes
the module, either. Thus this might be regarded as a security
issue.
4. The command ‘mcron -s file’, where /s/ takes an optional
numeric value, errors out.
3) The third patch fixes up the /getopt-long/ module to pass all the
new tests, as well as all of the existing ones (with the single
exception outlined in Point 2.3 above). Considering that the
entire Guile build also depends on /getopt-long/, we can have
some confidence that the changes do not bring any incompatibility
with existing code.
4) The final patch fixes up various commentary and doc-strings in
the code to emphasise the importance of predicates on optional
values, and generally make things more concrete.
next reply other threads:[~2020-04-19 17:47 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-19 17:47 Dale Mellor [this message]
2020-05-07 17:01 ` bug#40719: [PATCH 1/4] test: augment testing of (ice-9 getopt-long) module Dale Mellor
2020-05-07 17:03 ` bug#40719: [PATCH 2/4] test *broken*: augmented tests of (ice-9 getopt-long) Dale Mellor
2020-05-07 17:04 ` bug#40719: [PATCH 3/4] (ice-9 getopt-long): substantially re-written to pass all the new tests Dale Mellor
2020-05-07 17:05 ` bug#40719: [PATCH 4/4] (ice-9 getopt-long): update commentary and doc-strings Dale Mellor
2020-08-02 10:41 ` bug#40719: Patch set superseded by 42669 Dale Mellor
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/guile/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c991d9c48eef80a68274ee28a0c1040b4eea4953.camel@rdmp.org \
--to=guile-qf1qmg@rdmp.org \
--cc=40719@debbugs.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).