Re: [PATCH] Implementing IncludeCmd (feature request #17)
On Thu, Aug 31, 2023 at 10:58:27AM +0200, Michiel van den Heuvel wrote:
so if you think it would be a good use of *your* time, then go ahead.
After some somewhat more synchronous communication, it might be.
are you asking for a chat channel?
i'm @ossi:kde.org on matrix, ossi on libera.chat irc (if the matrix
bridge feels like working), and @ossilator on various social media and
chat sites.
i could create an official project channel (on matrix?), but
historically it seems a bit ... superfluous.
On the subject of more things I could do, is there a way to reproduce
the 2 regressions?
congrats, you forced me to actually research what the regressions are.
*grumble* :-D
for the imap thing, i suspect any mail with a large enough attachment
will do. maybe the imap compress extension needs to be in use, and/or
tls. no idea.
(possibly) related threads:
https://sourceforge.net/p/isync/mailman/isync-devel/thread/Y00lhIm7VdrJzG/D%40ugly/#msg37721793
https://sourceforge.net/p/isync/mailman/isync-devel/thread/87h740x2xe.fsf%40wavexx.thregr.org/#msg37675548
https://sourceforge.net/p/isync/mailman/isync-devel/thread/87edk45p9o.fsf%40b3l.xyz/#msg37883904
for the other problem, there is
https://sourceforge.net/p/isync/mailman/isync-devel/thread/f4d61b60-7a93-6b78-90d1-b96b285caa9c%40quaternum.net/#msg37708463
which contains a speculative analysis.
there is https://sourceforge.net/p/isync/bugs/69/ which may be actually
the same thing (sounds similar, but i'm not going to think about it at
this time of day). it has a patch attached, which is certainly
under-tested, and needs a sanity check, possibly resulting in a complete
rewrite.
speaking of tested, if you _really_ wanted to go overboard, you could
add a proper unit testing suite. so far only the core syncing algo is
tested, in a way that is really an integration test (which has the
advantage of testing a bit more as a side effect, but it's slow and
inelegant). this would also nicely fit with formalizing the error
reporting.
Subject: [PATCH] Add IncludeCmd directive to config parser
+static char *
+check_excess_tokens( conffile_t *cfile )
+{
+ char *arg = NULL;
+
i generally prefer early returns (and even goto) over temporaries.
int
getcline( conffile_t *cfile )
{
...
+ else if ((arg = check_excess_tokens( cfile )))
+ conf_error( cfile, "excess token '%s', not executing
"
+ "potentially malformed command\n",
arg );
my thinking was that the error message would be printed already by the
check function (hence "check", not "get"). i don't think the extra
verbosity about the specific consequence adds all that much value.
then the check function can just return an int.
oh, one thing i totally failed to notice: this is a noteworthy new
feature, so it should get an entry in NEWS. something like "Added
support for scripting arbitrary parts of the config file.", before the
currently last two entries, i think.
and you should add yourself to the end of the main contributor list in
AUTHORS.
regards
___
isync-devel mailing list
isync-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/isync-devel
Re: [PATCH] Implementing IncludeCmd (feature request #17)
> that depends on whose perspective you're assuming here.
> i'm likely not going to get it done anytime soon. pushing patches on me
> prompts me into action, so it's good for the project (not so much for my
> other projects, but the trade-off isn't that bad, as i'm actually much
> more efficient at reviewing than hacking myself).
> so if you think it would be a good use of *your* time, then go ahead.
After some somewhat more synchronous communication, it might be. This
project has been a nice refresher course on C.
> i implied that you need to do that anyway to handle things properly.
> also note that you'd save the duplicated check for the command.
>
> but looking at it again, duplication probably isn't even necessary: you
> can factor out check_excess_tokens() and make it also return a value.
I hope this is what you meant.
> the first patch is now Perfect (TM), so no need to re-send it.
Nice! Progress.
On the subject of more things I could do, is there a way to reproduce
the 2 regressions? I am not familiar with IMAP, but do know how to fix
a regression. I'm not in any hurry, and do not want to promise
anything. But I'd like to take a look.
Cheers, Michiel>From 8f4c551449e90f80cdbf31fd6a0b207e9802c3e7 Mon Sep 17 00:00:00 2001
From: Michiel van den Heuvel
Date: Thu, 17 Aug 2023 20:25:51 +0200
Subject: [PATCH] Add IncludeCmd directive to config parser
---
src/config.c | 88
src/config.h | 3 ++
src/mbsync.1 | 8 +
3 files changed, 93 insertions(+), 6 deletions(-)
diff --git a/src/config.c b/src/config.c
index ac90c25..a7476a9 100644
--- a/src/config.c
+++ b/src/config.c
@@ -61,12 +61,21 @@ expand_strdup( const char *s, const conffile_t *cfile )
}
}
+static void
+conf_print_loc( const conffile_t *cfile )
+{
+ if (cfile->eval_fp)
+ fprintf( stderr, "%s:%d:included:%d: ", cfile->file, cfile->line, cfile->eval_line );
+ else
+ fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+}
+
void
conf_error( conffile_t *cfile, const char *fmt, ... )
{
va_list va;
- fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+ conf_print_loc( cfile );
va_start( va, fmt );
vfprintf( stderr, fmt, va );
va_end( va );
@@ -79,7 +88,7 @@ conf_sys_error( conffile_t *cfile, const char *fmt, ... )
va_list va;
int errno_bak = errno;
- fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+ conf_print_loc( cfile );
errno = errno_bak;
va_start( va, fmt );
vsys_error( fmt, va );
@@ -318,17 +327,72 @@ getopt_helper( conffile_t *cfile, int *cops, channel_conf_t *conf )
return 1;
}
+static void
+eval_cmd_popen( conffile_t *cfile, const char *cmd )
+{
+ if (!(cfile->eval_fp = popen( cmd, "r" ))) {
+ sys_error( "popen" );
+ cfile->err = 1;
+ return;
+ }
+ cfile->eval_line = 0;
+ cfile->eval_command = nfstrdup( cmd );
+}
+
+static void
+eval_cmd_pclose( conffile_t *cfile )
+{
+ int ret;
+
+ if ((ret = pclose( cfile->eval_fp ))) {
+ if (ret < 0) {
+ sys_error( "pclose" );
+ cfile->err = 1;
+ } else if (WIFSIGNALED( ret )) {
+ conf_error( cfile, "command \"%s\" crashed with signal %d\n",
+ cfile->eval_command, WTERMSIG( ret ) );
+ } else {
+ conf_error( cfile, "command \"%s\" exited with status %d\n",
+ cfile->eval_command, WEXITSTATUS( ret ) );
+ }
+ }
+ free( cfile->eval_command );
+ cfile->eval_fp = NULL;
+ cfile->eval_command = NULL;
+}
+
+static int
+read_cline( conffile_t *cfile )
+{
+ if (cfile->eval_fp) {
+ cfile->eval_line++;
+ if ((cfile->rest = fgets( cfile->buf, cfile->bufl, cfile->eval_fp )))
+ return 1;
+ eval_cmd_pclose( cfile );
+ }
+ cfile->line++;
+ return (cfile->rest = fgets( cfile->buf, cfile->bufl, cfile->fp )) != NULL;
+}
+
+static char *
+check_excess_tokens( conffile_t *cfile )
+{
+ char *arg = NULL;
+
+ if (cfile->rest)
+ arg = get_arg( cfile, ARG_OPTIONAL, NULL );
+ return arg;
+}
+
int
getcline( conffile_t *cfile )
{
char *arg;
int comment;
- if (cfile->rest && (arg = get_arg( cfile, ARG_OPTIONAL, NULL )))
+ if ((arg = check_excess_tokens( cfile )))
conf_error( cfile, "excess token '%s'\n", arg );
- while (fgets( cfile->buf, cfile->bufl, cfile->fp )) {
- cfile->line++;
- cfile->rest = cfile->buf;
+ while (read_cline( cfile )) {
if (!(cfile->cmd = get_arg( cfile, ARG_OPTIONAL, ))) {
if (comment)
continue;
@@ -336,6 +400,16 @@ getcline( conffile_t *cfile )
}
if (!(cfile->val = get_arg( cfile, ARG_REQUIRED, NULL )))
continue;
+ if (!strcasecmp( cfile->cmd, "IncludeCmd" )) {
+ if (cfile->eval_fp)
+conf_error( cfile, "nested IncludeCmd\n" );
+ else if ((arg = check_excess_tokens( cfile )))
+conf_error( cfile, "excess token '%s', not executing "
+"potentially malformed command\n", arg );
+ else
+eval_cmd_popen( cfile, cfile->val );
+ continue;
+ }
return 1;
}
return 0;
@@ -488,6 +562,7 @@ load_config( const char *where )
return 1;
}
buf[sizeof(buf) - 1] = 0;
+
Re: [PATCH] Implementing IncludeCmd (feature request #17)
On Sat, Aug 26, 2023 at 02:09:49PM +0200, JWM van den Heuvel wrote:
The previous mail was me sending -- instead of saving -- a draft. My
bad.
thought so. no worries.
The perl scripts are a
work of art and make me want to punch something.
compliment accepted. :'-D
A nice future refactoring project. Looks good, but given the amount of
feedback my contributions require, not efficient if I take it up ;)
that depends on whose perspective you're assuming here.
i'm likely not going to get it done anytime soon. pushing patches on me
prompts me into action, so it's good for the project (not so much for my
other projects, but the trade-off isn't that bad, as i'm actually much
more efficient at reviewing than hacking myself).
so if you think it would be a good use of *your* time, then go ahead.
> + if (cfile->cmd && !strcasecmp( cfile->cmd, "IncludeCmd"
> )) {
> + if (cfile->include_fp)
> + conf_error( cfile, "nested IncludeCmd\n" );
> + else
> + include_cmd_popen( cfile, cfile->val );
> + }
wouldn't just moving the body into the conditional below work?
of course that would do the excess-argument check only after doing the
popen, but that's consistent with how it works for other options.
otoh, the side effects of instantly executing a potentially malformed
command may be somewhat more grave than when processing other options.
but handlign this properly would actually require a separate check
anyway, as then we need to actually skip the command.
The problem with doing it after the popen is that `conf_print_loc`
will then show an error as if it is in the included output while it
is in the file itself.
right.
Moving it would mean copying the excess token
detection code, which is not worth it IMHO.
i implied that you need to do that anyway to handle things properly.
also note that you'd save the duplicated check for the command.
but looking at it again, duplication probably isn't even necessary: you
can factor out check_excess_tokens() and make it also return a value.
the first patch is now Perfect (TM), so no need to re-send it.
regards
___
isync-devel mailing list
isync-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/isync-devel
Re: [PATCH] Implementing IncludeCmd (feature request #17)
Hi Oswald,
The previous mail was me sending -- instead of saving -- a draft. My bad.
Anyway, here are the updated patches.
> i'm attaching it for your masochistic pleasure. there you can see that
> it's actually not even aimed at isync specifically, because c++/qt?! i
> originally wrote this some 22 years ago, when astyle was Teh Shit.
> i wouldn't expect it to actually produce invariably correct results, and
> haven't used it on isync code for ... a very long time.
Ah ok, I'll just keep formatting by hand then. The perl scripts are a
work of art and make me want to punch something. Probably not cards ;)
> depending on what you mean by "this".
>
> if you mean the "short" tangent of using a temporary: nope, not worth
> it.
>
> if you mean the "long" tangent of using xprintf: yes, but not in this
> patch series.
> for background, see the wip/better-stderr branch, which turns the
> personal aversion into an actual requirement. but notably, the config
> parser doesn't strictly _need_ that, because it runs before concurrency
> starts.
> once the infra is there, strerror(errno) (and thus perror()) can be
> handled via the %m format (stolen from syslog), so things can be unified
> pretty much. the branch shows the direction.
A nice future refactoring project. Looks good, but given the amount of
feedback my contributions require, not efficient if I take it up ;)
Thanks for the patience and guidance!
> no, i mean the saving of errno - pedantically, you need to wrap the
> first fprintf().
> otoh, if the first print fails, the second one will most likely fail as
> well, and therefore it doesn't really matter if errno gets corrupted or
> not. but as i pushed e295f483 after much deliberation, i guess we should
> keep up the good practice.
Done!
> > +static int
> > +read_cline( conffile_t *cfile )
> > +{
> > + if (cfile->include_fp) {
> > + cfile->include_line++;
> > + if ((cfile->rest = fgets( cfile->buf, cfile->bufl,
> > cfile->include_fp )))
>
> assigning cfile->rest right at the start would be less obscure. the
> pointer value is already known anyway.
That gets you an excess token error on (/after) the last line. It's not
about setting it to `buf`, but only doing it if there is actually a line
left to read.
> > + if (cfile->cmd && !strcasecmp( cfile->cmd, "IncludeCmd" )) {
> > + if (cfile->include_fp)
> > + conf_error( cfile, "nested IncludeCmd\n" );
> > + else
> > + include_cmd_popen( cfile, cfile->val );
> > + }
>
> wouldn't just moving the body into the conditional below work?
> of course that would do the excess-argument check only after doing the
> popen, but that's consistent with how it works for other options.
> otoh, the side effects of instantly executing a potentially malformed
> command may be somewhat more grave than when processing other options.
> but handlign this properly would actually require a separate check
> anyway, as then we need to actually skip the command.
The problem with doing it after the popen is that `conf_print_loc`
will then show an error as if it is in the included output while it
is in the file itself. Moving it would mean copying the excess token
detection code, which is not worth it IMHO.
Cheers!>From c8d835e2e3120b7ab562836ed0d21079a86e2809 Mon Sep 17 00:00:00 2001
From: Michiel van den Heuvel
Date: Thu, 10 Aug 2023 05:20:55 +0200
Subject: [PATCH 1/2] Refactor printing configuration errors
In preparation for adding the output of commands as configuration lines,
which will complicate printing.
---
src/config.c | 117 +-
src/config.h | 3 ++
src/driver.c | 6 +--
src/drv_imap.c| 78 ---
src/drv_maildir.c | 18 +++
5 files changed, 94 insertions(+), 128 deletions(-)
diff --git a/src/config.c b/src/config.c
index 456bd47..ac90c25 100644
--- a/src/config.c
+++ b/src/config.c
@@ -61,6 +61,32 @@ expand_strdup( const char *s, const conffile_t *cfile )
}
}
+void
+conf_error( conffile_t *cfile, const char *fmt, ... )
+{
+ va_list va;
+
+ fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+ va_start( va, fmt );
+ vfprintf( stderr, fmt, va );
+ va_end( va );
+ cfile->err = 1;
+}
+
+void
+conf_sys_error( conffile_t *cfile, const char *fmt, ... )
+{
+ va_list va;
+
+ int errno_bak = errno;
+ fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+ errno = errno_bak;
+ va_start( va, fmt );
+ vsys_error( fmt, va );
+ va_end( va );
+ cfile->err = 1;
+}
+
char *
get_arg( conffile_t *cfile, int required, int *comment )
{
@@ -75,10 +101,8 @@ get_arg( conffile_t *cfile, int required, int *comment )
if (!c || c == '#') {
if (comment)
*comment = (c == '#');
- if (required) {
- error( "%s:%d: parameter missing\n", cfile->file, cfile->line );
- cfile->err = 1;
- }
+ if (required)
+ conf_error(
Re: [PATCH] Implementing IncludeCmd (feature request #17)
Quoting Oswald Buddenhagen (2023-08-22 15:51:20)
> On Tue, Aug 22, 2023 at 12:42:25PM +0200, Michiel van den Heuvel wrote:
> >> >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'll trade you my magnetic needles so you can do it directly on disk
> >next time. ;)
> >
> :-)=)
>
> >Maybe just add it to the repository?
> >
> no, it feels "too off-topic".
> i'm attaching it for your masochistic pleasure. there you can see that
> it's actually not even aimed at isync specifically, because c++/qt?! i
> originally wrote this some 22 years ago, when astyle was Teh Shit.
> i wouldn't expect it to actually produce invariably correct results, and
> haven't used it on isync code for ... a very long time.
>
> >> >+ 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 ...
> >
> >Not fixed yet due to 1: the next point, and 2: do you want this?
> >
> depending on what you mean by "this".
>
> if you mean the "short" tangent of using a temporary: nope, not worth
> it.
>
> if you mean the "long" tangent of using xprintf: yes, but not in this
> patch series.
> for background, see the wip/better-stderr branch, which turns the
> personal aversion into an actual requirement. but notably, the config
> parser doesn't strictly _need_ that, because it runs before concurrency
> starts.
>
> >It would
> >introduce some duplication because conf_error and conf_sys_error print
> >to stderr and a buffer respectively. But if you see a way to avoid
> >this,
> >I'd like to hear it.
> >
> once the infra is there, strerror(errno) (and thus perror()) can be
> handled via the %m format (stolen from syslog), so things can be unified
> pretty much. the branch shows the direction.
>
> >> >+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.
> >
> >Sorry, my lack of experience with C is showing here. I looked at
> >vsys_error, but are not sure what you mean. Is it the `abort`? Do I fix
> >it with inlining `vsys_error` and a dynamically allocated buffer?
> >
> no, i mean the saving of errno - pedantically, you need to wrap the
> first fprintf().
> otoh, if the first print fails, the second one will most likely fail as
> well, and therefore it doesn't really matter if errno gets corrupted or
> not. but as i pushed e295f483 after much deliberation, i guess we should
> keep up the good practice.
>
> >Do you think that the `pclose` branch `ret < 0` error shouldn't be a
> >`conf_sys_error` but the normal one (`sys_error`)?
> >
> yeah, probably. and definitely it should match the popen one.
>
> >+static int
> >+read_cline( conffile_t *cfile )
> >+{
> >+ if (cfile->include_fp) {
> >+ cfile->include_line++;
> >+ if ((cfile->rest = fgets( cfile->buf, cfile->bufl,
> > cfile->include_fp )))
>
> assigning cfile->rest right at the start would be less obscure. the
> pointer value is already known anyway.
That gets you an excess token error on (/after) the last line. It's not
about setting it to `buf`, but only doing it if there is actually a line
left to read.
> >@@ -325,9 +377,13 @@ getcline( conffile_t *cfile )
> >
> >+ if (cfile->cmd && !strcasecmp( cfile->cmd, "IncludeCmd" )) {
> >+ if (cfile->include_fp)
> >+ conf_error( cfile, "nested IncludeCmd\n" );
> >+ else
> >+ include_cmd_popen( cfile, cfile->val );
> >+ }
> >
> wouldn't just moving the body into the conditional below work?
> of course that would do the excess-argument check only after doing the
> popen, but that's consistent with how it works for other options.
> otoh, the side effects of instantly executing a potentially malformed
> command may be somewhat more grave than when processing other options.
> but
Re: [PATCH] Implementing IncludeCmd (feature request #17)
On Tue, Aug 22, 2023 at 12:42:25PM +0200, Michiel van den Heuvel wrote:
>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'll trade you my magnetic needles so you can do it directly on disk
next time. ;)
:-)=)
Maybe just add it to the repository?
no, it feels "too off-topic".
i'm attaching it for your masochistic pleasure. there you can see that
it's actually not even aimed at isync specifically, because c++/qt?! i
originally wrote this some 22 years ago, when astyle was Teh Shit.
i wouldn't expect it to actually produce invariably correct results, and
haven't used it on isync code for ... a very long time.
>+ 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 ...
Not fixed yet due to 1: the next point, and 2: do you want this?
depending on what you mean by "this".
if you mean the "short" tangent of using a temporary: nope, not worth
it.
if you mean the "long" tangent of using xprintf: yes, but not in this
patch series.
for background, see the wip/better-stderr branch, which turns the
personal aversion into an actual requirement. but notably, the config
parser doesn't strictly _need_ that, because it runs before concurrency
starts.
It would
introduce some duplication because conf_error and conf_sys_error print
to stderr and a buffer respectively. But if you see a way to avoid
this,
I'd like to hear it.
once the infra is there, strerror(errno) (and thus perror()) can be
handled via the %m format (stolen from syslog), so things can be unified
pretty much. the branch shows the direction.
>+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.
Sorry, my lack of experience with C is showing here. I looked at
vsys_error, but are not sure what you mean. Is it the `abort`? Do I fix
it with inlining `vsys_error` and a dynamically allocated buffer?
no, i mean the saving of errno - pedantically, you need to wrap the
first fprintf().
otoh, if the first print fails, the second one will most likely fail as
well, and therefore it doesn't really matter if errno gets corrupted or
not. but as i pushed e295f483 after much deliberation, i guess we should
keep up the good practice.
Do you think that the `pclose` branch `ret < 0` error shouldn't be a
`conf_sys_error` but the normal one (`sys_error`)?
yeah, probably. and definitely it should match the popen one.
Subject: [PATCH 1/2] Refactor printing configuration errors
--- a/src/config.c
+++ b/src/config.c
@@ -61,6 +61,31 @@ expand_strdup( const char *s, const conffile_t *cfile )
+void
+conf_error( conffile_t *cfile, const char *fmt, ... )
+{
+ va_list va;
+
+ flushn();
bzzt
Subject: [PATCH 2/2] Add IncludeCmd directive to config parser
--- a/src/config.c
+++ b/src/config.c
void
conf_error( conffile_t *cfile, const char *fmt, ... )
{
va_list va;
- flushn();
ah, you committed it in the wrong patch. ^^
+static void
+include_cmd_pclose( conffile_t *cfile )
+{
...
+ free( cfile->include_command );
should null that pointer for safety/pedantry as well.
+static int
+read_cline( conffile_t *cfile )
+{
+ if (cfile->include_fp) {
+ cfile->include_line++;
+ if ((cfile->rest = fgets( cfile->buf, cfile->bufl,
cfile->include_fp )))
assigning cfile->rest right at the start would be less obscure. the
pointer value is already known anyway.
+ return 1;
+ include_cmd_pclose( cfile );
+ }
+ cfile->line++;
+ return (cfile->rest = fgets( cfile->buf, cfile->bufl, cfile->fp )) !=
NULL;
...
@@ -325,9 +377,13 @@ getcline( conffile_t *cfile )
+ if (cfile->cmd && !strcasecmp( cfile->cmd, "IncludeCmd" )) {
+ if (cfile->include_fp)
+ conf_error( cfile, "nested IncludeCmd\n" );
+ else
+
Re: [PATCH] Implementing IncludeCmd (feature request #17)
Hi!
Once again, a set of updated patches.
> 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.)
That might've been a bit too implicit for me. ;) Changed it to IncludeCmd.
>
> >> 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.
Spent some more time testing. Everything looks good to me.
> >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. ;-)
I'll trade you my magnetic needles so you can do it directly on disk
next time. ;)
Maybe just add it to the repository?
> >(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
Yeah, on that note, thanks again for all the patience!
> >+ 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 ...
Not fixed yet due to 1: the next point, and 2: do you want this? It would
introduce some duplication because conf_error and conf_sys_error print
to stderr and a buffer respectively. But if you see a way to avoid this,
I'd like to hear it.
>
> >+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.
Sorry, my lack of experience with C is showing here. I looked at
vsys_error, but are not sure what you mean. Is it the `abort`? Do I fix
it with inlining `vsys_error` and a dynamically allocated buffer?
> signal %d",... WTERMSIG().
> pedantically, that quoting is insufficient because it doesn't do
> escaping, but there's no need to overdo it here.
That's exactly the reason my inner pedant thought it was best to use
the previous version. But, changed it.
> 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).
Yes. Because the error message needs to show the correct line
(`include_fp` must still be null when printing the error message)
I reorganized the code a bit.
> 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.
Well, I now put your suggestion verbatim at the bottom. Choices...
Do you think that the `pclose` branch `ret < 0` error shouldn't be a
`conf_sys_error` but the normal one (`sys_error`)?
Cheers!>From 5c47b627095a5b69c7096f667dfd441f86e163d6 Mon Sep 17 00:00:00 2001
From: Michiel van den Heuvel
Date: Thu, 10 Aug 2023 05:20:55 +0200
Subject: [PATCH 1/2] Refactor printing configuration errors
In preparation for adding the output of commands as configuration lines,
which will complicate printing.
---
src/config.c | 116 +-
src/config.h | 3 ++
src/driver.c | 6 +--
src/drv_imap.c| 78 ---
src/drv_maildir.c | 18 +++
5 files changed, 93 insertions(+), 128 deletions(-)
diff --git a/src/config.c
Re: [PATCH] Implementing IncludeCmd (feature request #17)
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" ))) {
+
Re: [PATCH] Implementing IncludeCmd (feature request #17)
Hi Oswald,
> > It might be worthwhile to set `cfile->error` here too, and
> > I'll gladly amend the patch if you agree.
>
> yeah, i'd do that.
Done!
> theoretically yes, though in practice i'd recommend that users just call
> a script, because the extra layer of quoting/escaping makes things ugly
> and easy to get wrong. add it if you can't help yourself. ;)
> so just call a script already! ;)
Oh you should see my personal configuration. So many inline scripts
under multiple layers of escaping. But, with my own troubles in getting
a clean implementation plus the feedback, I think I can help myself ;)
External scripts it is.
> eval => included, i guess.
> also, that space afterwards should be probably a colon, otherwise it's
> kinda confusing and weird-looking.
Changed it, but are you sure? Now that the command is called `Eval`? The
space *was* weird-looking, thanks. I was already unhappy about it.
> on that matter, have you tested all error scenarios?
The ones I added? Or all that were modified? 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.
> > +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?
> ... but here it gets "interesting":
> seeing the command after dequoting may be actually helpful, especially
> when multi-line commands are supported. though of course this can be
> ridiculously long. but i guess we'll get away with just qouting it.
Added this.
> > +char *
> > +read_cline( conffile_t *cfile )
>
> i think that should be just an int.
Fixed (?) by comparing it to `NULL` when returning. Also made it less
convoluted.
> >+.SS Dynamic options
>
> why a separate section? it doesn't really fit the pattern for the *Cmd
> commands.
I didn't really know where to put it. It can occur literally everywhere,
which no other options (like the `*Cmd`) do, which is why I did not add
it there. This time I put it next to the global options.
> >+void
> >+conf_error( const conffile_t *cfile, const char* fmt, ... )
>
> second asterisk has space on the wrong side.
> this repeats for several other string arguments.
> but i think this is actually the only whitespace mistake you've made,
> which may be a first among non-trivial external isync contributions
> during my "reign". ^^
Hehehe :) Nice trophy. I don't suppose you have some formatter config
file?
(Hopefully) All your other proposed changes were incorporated in these
revised patches.
Cheers!>From 92cc071aeb14cab302eb0db206a30e2c5c78623b Mon Sep 17 00:00:00 2001
From: Michiel van den Heuvel
Date: Thu, 10 Aug 2023 05:20:55 +0200
Subject: [PATCH 1/2] Refactor printing configuration errors
In preparation for adding the output of commands as configuration lines,
which will complicate printing.
---
src/config.c | 106 +-
src/config.h | 3 ++
src/driver.c | 6 +--
src/drv_imap.c| 46 +++-
src/drv_maildir.c | 9 ++--
5 files changed, 72 insertions(+), 98 deletions(-)
diff --git a/src/config.c b/src/config.c
index 456bd47..0de2cb2 100644
--- a/src/config.c
+++ b/src/config.c
@@ -61,6 +61,31 @@ expand_strdup( const char *s, const conffile_t *cfile )
}
}
+void
+conf_error( conffile_t *cfile, const char *fmt, ... )
+{
+ va_list va;
+
+ flushn();
+ fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+ va_start( va, fmt );
+ vfprintf( stderr, fmt, va );
+ va_end( va );
+ cfile->err = 1;
+}
+
+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 );
+ va_end( va );
+ cfile->err = 1;
+}
+
char *
get_arg( conffile_t *cfile, int required, int *comment )
{
@@ -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" );
}
ret = NULL;
} else {
@@ -98,13 +122,11 @@ get_arg( conffile_t *cfile, int required, int *comment )
}
*t = 0;
if (escaped) {
- error( "%s:%d: unterminated escape sequence\n", cfile->file, cfile->line );
- cfile->err = 1;
+ conf_error( cfile, "unterminated escape sequence\n" );
ret = NULL;
}
if (quoted) {
- error( "%s:%d: missing closing quote\n", cfile->file, cfile->line );
- cfile->err = 1;
+ conf_error( cfile, "missing closing quote\n" );
ret = NULL;
}
}
@@ -124,9 +146,7 @@
Re: [PATCH] Implementing IncludeCmd (feature request #17)
hi!
On Thu, Aug 17, 2023 at 10:59:48AM +0200, Michiel van den Heuvel wrote:
I named the command `Eval` instead of `IncludeCmd` because it seemed
clearer to me, but you're welcome to change that.
my thinking was that IncludeCmd would nicely complement a hypothetical
Include command, following the structure of the pre-existing *Cmd
commands.
the code needs no renaming, eval is nice and short.
Patch 1 was necessary to print error line numbers correctly after adding
`Eval` without modifying every line where errors are printed.
It might be worthwhile to set `cfile->error` here too, and I'll gladly
amend the patch if you agree.
yeah, i'd do that.
On the other hand, it is a formatting function...
nah, the name doesn't say so.
Patch 2 is where the actual work happens. I indeed tried to follow
`cred_from_cmd`.
Nesting `Eval` statements is disallowed. Implementing
this would certainly be doable, but I don't really see the point.
agreed.
I was working on an extra patch, which would extend the parser to
allow multiline values for slightly longer commands. [...]
- Do you see merit in this feature? I don't currently need it, but
like configuration file formats which offer possibilities to split
lines if they get too long.
theoretically yes, though in practice i'd recommend that users just call
a script, because the extra layer of quoting/escaping makes things ugly
and easy to get wrong. add it if you can't help yourself. ;)
- If so, what would you think is the better implementation? Just make
the buffer really large (what is large?) and error if a script is
longer? The current line limit is 1024. [...]
i think the static 1k buffer is just fine - that amounts to a script
with up to ~20 lines, and it seems a tad insane to inline more than
that.
- Do you agree with the syntax? This still requires backslashes even
when quoted, which I like due to the missing closing quote error
which would otherwise be hard to correctly detect.
i guess. it also has the advantage of somewhat signaling the fact of the
extra layer of quoting.
- What should the behaviour be when not quoted?
A literal newline (current implementation)?
that seems convenient, but it violates expectations about how line
continuations usually behave, regardless of whether quoted or not.
Read it as whitespace?
that would be more conventional. it would of course also mean that users
need to include `&&` (or `;`) to chain the commands, which many would
probably get wrong on first try.
the alternative would be more like a here-doc, and would warrant a
corresponding syntax. seems like mild overkill, though.
so just call a script already! ;)
Subject: [PATCH 2/2] Add Eval directive to config parser
--- a/src/config.c
+++ b/src/config.c
+void
static
+conf_print_loc( const conffile_t *cfile )
+{
+ if (cfile->eval_fp != NULL) {
i treat pointers a booleans in conditionals.
also, excess braces.
+ fprintf( stderr, "%s:%d:eval %d: ", cfile->file, cfile->line,
cfile->eval_line );
eval => included, i guess.
also, that space afterwards should be probably a colon, otherwise it's
kinda confusing and weird-looking.
on that matter, have you tested all error scenarios?
+void
should return an int (zero is error), which should lead to early
termination.
also, static.
+eval_popen( conffile_t *cfile, const char *cmd )
+{
+ if (*cmd == '+') {
+ flushn();
+ cmd++;
+ }
that makes no sense here - unlike the credential commands, this is
executed at program startup where no progress info has been printed yet.
don't forget to prune it from the docu as well.
+ if (!(cfile->eval_fp = popen( cmd, "r" ))) {
+ sys_error( "%s failed", cmd );
i think it makes no sense to print the command here, as failure here
would be a fork() failure or something like that. so just "popen" should
do.
(though i wonder how things would look like in an msys build.)
+void
as above.
+eval_pclose( conffile_t *cfile )
+{
+ int ret;
+
+ ret = pclose( cfile->eval_fp );
+ cfile->eval_fp = NULL;
+ if (ret) {
+ if (ret < 0)
+ conf_sys_error( cfile, "command failed" );
+ else if (WIFSIGNALED( ret ))
+ conf_error( cfile, "command crashed\n" );
+ else
+ conf_error( cfile, "command exited with status %d\n",
WEXITSTATUS( ret ) );
... but here it gets "interesting":
seeing the command after dequoting may be actually helpful, especially
when multi-line commands are supported. though of course this can be
ridiculously long. but i guess we'll get away with just qouting it.
here-doc syntax would make printing the command superfluous, because it
would be used verbatim anyway ... until someone actually implements
expandos (like %{Host}).
+char *
i think that should be just an int.
also, static.
