On Thu, Aug 17, 2023 at 08:27:51PM +0200, Michiel van den Heuvel wrote:
eval => included, i guess.
Changed it, but are you sure? Now that the command is called `Eval`?
well, you cut away the part where i kind of implied that it really
should be IncludeCmd. (it kind of is awkward, but i think consistency
trumps that.)
on that matter, have you tested all error scenarios?
The ones I added? Or all that were modified?
modified only to the degree that the change cannot be confidently
claimed to be irrelevant. so if you changed the output format or the
control flow, that would need testing.
On the ones that I added,
yes, except for failure of popen and the `ret < 0` case in `eval_pclose`.
I have to admit I do not really know how to trigger those.
yeah, that's kind of impossible without some error injection via ptrace
or some such. but you can fake the failure by just hacking the code. i
wouldn't bother with the 3rd pclose branch, but popen should be tested.
> +void
> +eval_popen( conffile_t *cfile, const char *cmd )
should return an int (zero is error), which should lead to early
termination.
It was missing the `cfile->err` plumbing. Which is fixed, assuming that's
what you meant, in the attached implementation. I could also return 0 from
getcline in that case, but that does not match the rest of the parser.
Is a failed popen that different?
i'm not sure ...
technically, the failed include could have structural consequences, as
the command may be intending to start a new section, which is continued
inline. otoh, it would be really stupid to write a config that does
that.
and we live with followup errors anyway, so accepting some more wouldn't
be a big deal. so i guess no change needed.
I don't suppose you have some formatter config file?
bah! Real Hackers (TM) write their own formatters as shell-perl hybrids
with recursive regular expressions. ON PUNCH CARDS.
(i can post it if you're really curious, but you need to sign a
liability waiver. ;-)
(Hopefully) All your other proposed changes were incorporated in these
revised patches.
mostly. but as it goes, the longer you look, the more you find. :-D
Subject: [PATCH 1/2] Refactor printing configuration errors
--- a/src/config.c
+++ b/src/config.c
+void
+conf_error( conffile_t *cfile, const char *fmt, ... )
+{
+ va_list va;
+
+ flushn();
that's unnecessary, for the same reason that +cmd is.
+ fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+ va_start( va, fmt );
+ vfprintf( stderr, fmt, va );
totally on a tangent ...
i have a bit of an aversion towards tearing apart format strings. it
isn't a big deal here, because we're just printing to a stream with no
concurrency, but if the target was syslog or a dynamically allocated
string, things would look differently. this could be avoided by printing
the "payload" to a temporary, but this would obviously have a runtime
cost, esp. when using nfasprintf() to be super-safe. but as we already
have xvprintf_core(), this overhead could be avoided by supporting
recursive format strings: xprintf("%s:%d: %@\n", file, line, fmt, va).
of course this would be total overkill here ... though once you have it,
all kinds of use cases pop up. just thinking aloud ...
+void
+conf_sys_error( conffile_t *cfile, const char *fmt, ... )
+{
+ va_list va;
+
+ fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+ va_start( va, fmt );
+ vsys_error( fmt, va );
that call is theoretically unsafe; see the implementation.
@@ -76,8 +101,7 @@ get_arg( conffile_t *cfile, int required, int *comment )
if (comment)
*comment = (c == '#');
if (required) {
- error( "%s:%d: parameter missing\n", cfile->file,
cfile->line );
- cfile->err = 1;
+ conf_error( cfile, "parameter missing\n" );
}
the braces are now excessive.
this repeats in a few places, in some of which it will probably have a
cascading effect due to the brace symmetry requirement.
--- a/src/config.h
+++ b/src/config.h
@@ -29,6 +29,9 @@ extern char FieldDelimiter;
char *expand_strdup( const char *s, const conffile_t *cfile );
+void ATTR_PRINTFLIKE(2, 3) conf_error( conffile_t *, const char *, ... );
+void ATTR_PRINTFLIKE(2, 3) conf_sys_error( conffile_t *, const char *, ... );
+
i'd probably put this above expand_strdup(), simply because it would
look better due to the macros not standing out that much.
oh, and you pointed out to me that i'm being inconsistent about
including parameter names in the print/error function prototypes. that
omission is a mistake and should not be copied.
Subject: [PATCH 2/2] Add Eval directive to config parser
--- a/src/config.c
+++ b/src/config.c
+static void
+eval_popen( conffile_t *cfile, const char *cmd )
+{
+ if (!(cfile->eval_fp = popen( cmd, "r" ))) {
+ sys_error( "popen failed" );
+ cfile->err = 1;
return here, to avoid leak.
+ }
+ cfile->eval_line = 0;
+ cfile->eval_command = nfstrdup( cmd );
+}
+
+static void
+eval_pclose( conffile_t *cfile )
+{
+ int ret;
+
+ ret = pclose( cfile->eval_fp );
+ cfile->eval_fp = NULL;
i'd probably delay that, so i could put the assignment into the if().
+ if (ret) {
+ if (ret < 0)
+ conf_sys_error( cfile, "command failed: %s",
cfile->eval_command );
that's probably just a "pclose failed".
and come to think of it, the "failed" suffix is unconventional -
perror() is generally just called with the function name if no more
specific error message is provided.
+ else if (WIFSIGNALED( ret ))
+ conf_error( cfile, "command crashed: %s\n",
cfile->eval_command );
this isn't going to look right. more like "command \"%s\" crashed with
signal %d",... WTERMSIG().
pedantically, that quoting is insufficient because it doesn't do
escaping, but there's no need to overdo it here.
+ else
+ conf_error( cfile, "command exited with status %d:
%s\n",
+ WEXITSTATUS( ret ), cfile->eval_command );
... and here the same structure.
+static int
+read_cline( conffile_t *cfile )
+{
+ if (cfile->eval_fp) {
+ cfile->eval_line++;
+ if (fgets( cfile->buf, cfile->bufl, cfile->eval_fp ))
+ return 1;
+ else
use no `else` after jump statements, except to maintain symmetry.
+ eval_pclose( cfile );
+ }
+ cfile->line++;
+ return fgets( cfile->buf, cfile->bufl, cfile->fp ) != NULL;
+}
+
int
getcline( conffile_t *cfile )
{
@@ -327,14 +380,23 @@ getcline( conffile_t *cfile )
if (cfile->rest && (arg = get_arg( cfile, ARG_OPTIONAL, NULL )))
conf_error( cfile, "excess token '%s'\n", arg );
- while (fgets( cfile->buf, cfile->bufl, cfile->fp )) {
- cfile->line++;
+ while (read_cline( cfile )) {
cfile->rest = cfile->buf;
this keeps bothering me, so ...
it would probably make sense to move this into read_cline().
if (!(cfile->cmd = get_arg( cfile, ARG_OPTIONAL, &comment ))) {
if (comment)
continue;
return 1;
}
+ if (!strcmp( cfile->cmd, "Eval" )) {
strcasecmp()
"IncludeCmd"
+ if ((arg = get_arg( cfile, ARG_REQUIRED, NULL ))) {
+ if (cfile->eval_fp) {
+ conf_error( cfile, "nested eval\n" );
"IncludeCmd"
+ continue;
+ }
+ eval_popen( cfile, arg );
+ }
+ continue;
this isn't quite right; it would miss excessive arguments.
you can probably use `return getcline()` (and rely on the compiler
eliding the tail recursion).
--- a/src/mbsync.1
+++ b/src/mbsync.1
come to think of it, you were probably right to make a separate section
for it. but it may be better to put it at the end, after the global one
- it's kind of more like the global options. otoh, it's kind of a
syntactic/structural part of the config, so having it at the top also
makes sense. hmm.
+\fBEval\fR \fIcommand\fR
+Specify a shell command to obtain options. The command can output zero
+or more lines which will be interpreted as if they appeared inline. This
+keyword is allowed in every section.
how about
.SS Dynamic Configuration
.TP
\fBIncludeCmd\fR \fIcommand\fR
Specify a shell command to obtain configuration file content from.
This keyword can appear anywhere. The command's output will be
interpreted as if it appeared inline; it can range from nothing at all
to multiple sections.
?
regards
_______________________________________________
isync-devel mailing list
isync-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/isync-devel
