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: Socket Error, Possible protocol violation
On Thu, Aug 17, 2023 at 07:59:09AM -0400, brittanderson--- via isync-devel wrote: The error has reoccured. It is: Socket error: receive buffer full. Probably protocol error. hmm, this might in fact be one of the two regressions that prevent me from releasing 1.5.0 - i honestly don't remember, as it's been over a year now - i'm sidetracked by other projects. Happy to provide more details if you can point me in the best direction. i don't think that's necessary. it "just" requires me looking really hard at the code. the culprit must be one of the last commits to socket.c (git-bisect'ing it would not hurt). if someone else wants to take a shot at analyzing it, that would be most welcome. regards ___ isync-devel mailing list isync-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/isync-devel
Re: Socket Error, Possible protocol violation
The error has reoccured. It is:
Socket error: receive buffer full. Probably protocol error.
running =mbsync -D university=
Gives an output that includes this snippet:
#+Begin_quote
N: Called get_uidnext, ret=1273
-> log: F 1 1273 (save UIDNEXT of near side)
-> log: # 192877 0 yMXnDcoTZKtm (new TUID)
N: Called get_memory_usage, ret=0
F: [ 9] Enter fetch_msg, uid=192877, want_flags=no, want_date=no
F: >>> 6 UID FETCH 192877 (BODY.PEEK[])
F: [ 9] Leave fetch_msg
F: [ 7] Callback leave load_box
F: * 3 FETCH (UID 192877 BODY[] {587687}
F: === (98304 bytes omitted)
F: === (65536 bytes omitted)
F: === (10 bytes omitted)
F: === (10 bytes omitted)
Socket error: receive buffer full. Probably protocol error.
#+End_quote
I cannot find anything to match the offending email on my (near) side.
When I log on to microsoft365 and deleted the most recent email from my
inbox the error went away.
Happy to provide more details if you can point me in the best direction.
Thank you, Britt
Oswald Buddenhagen writes:
> DuckDuckGo was unable to verify sender identity
>
> On Tue, Aug 15, 2023 at 02:31:11PM -0400, brittanderson--- via isync-devel
> wrote:
>>Oswald Buddenhagen writes:
>>> the two sides have different uids. the ones appearing in the imap
>>> stream don't match the local mailboxes. the complete output contains
>>> the local (or more precisely, near-side) uids as well, and their
>>> pairings with remote (far-side) ones.
>>
>>I saw the pairs, but there was no element with the same UID as the error
>>message about the socket error. Do the UID in the error reports come
>>from elsewhere as well?
>>
> you'd need to show some logs for me to have a clue what you're talking
> about.
>
>>For example, something related to davmail or the
>>microsoft exchange server I am talking to?
>>
> imap allows the server to include free-form text into error messages,
> so it's theoretically possible.
>
> regards
>
>
> ___
> isync-devel mailing list
> isync-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/isync-devel
___
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!
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.
[PATCH] Implementing IncludeCmd (feature request #17)
Hi!
It took some time, but here is an implementation of feature request
#17. I named the command `Eval` instead of `IncludeCmd` because it seemed
clearer to me, but you're welcome to change that.
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. On the other hand, it is a formatting function...
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.
I was working on an extra patch, which would extend the parser to
allow multiline values for slightly longer commands. For example:
Eval "\
echo User $USER \
echo Path $XDG_DATA_HOME/notmuch/default \
"
Due to the way the `cfile->buf` buffer is aliased by `cfile->cmd` etc
this proved to be a little bit more difficult than I initially thought. I
have a few questions:
- 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.
- 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. The other option I see is
dynamically reallocating the buffer (I have already played with this,
but sadly that implementation was not aware of comments) and updating
the pointers in `cfile` as needed. Maybe throw in a `restrict`.
- 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.
- What should the behaviour be when not quoted? A literal newline
(current implementation)? Read it as whitespace?
Anyway, valgrind was an issue on my end and is fixed. So it has been
tested with it, hopefully no leaks this time ;)
Once again, feedback is very welcome!
Kind regards,
Michiel van den Heuvel>From 1ef12e2b3f205af4324565183b700e27ecc88830 Mon Sep 17 00:00:00 2001
From: Michiel van den Heuvel
Date: Wed, 16 Aug 2023 20:36:48 +0200
Subject: [PATCH 2/2] Add Eval directive to config parser
---
src/config.c | 81 +---
src/config.h | 2 ++
src/mbsync.1 | 10 +++
3 files changed, 89 insertions(+), 4 deletions(-)
diff --git a/src/config.c b/src/config.c
index 3aa3f7c..6e08dd9 100644
--- a/src/config.c
+++ b/src/config.c
@@ -61,13 +61,23 @@ expand_strdup( const char *s, const conffile_t *cfile )
}
}
+void
+conf_print_loc( const conffile_t *cfile )
+{
+ if (cfile->eval_fp != NULL) {
+ fprintf( stderr, "%s:%d:eval %d: ", cfile->file, cfile->line, cfile->eval_line );
+ } else {
+ fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+ }
+}
+
void
conf_error( const conffile_t *cfile, const char* fmt, ... )
{
va_list va;
flushn();
- fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+ conf_print_loc( cfile );
va_start( va, fmt );
vfprintf( stderr, fmt, va );
va_end( va );
@@ -78,7 +88,7 @@ conf_sys_error( const conffile_t *cfile, const char* msg, ... )
{
va_list va;
- fprintf( stderr, "%s:%d: ", cfile->file, cfile->line );
+ conf_print_loc( cfile );
va_start( va, msg );
vsys_error( msg, va );
va_end( va );
@@ -326,6 +336,58 @@ getopt_helper( conffile_t *cfile, int *cops, channel_conf_t *conf )
return 1;
}
+void
+eval_popen( conffile_t *cfile, const char *cmd )
+{
+ if (*cmd == '+') {
+ flushn();
+ cmd++;
+ }
+ if (!(cfile->eval_fp = popen( cmd, "r" ))) {
+ sys_error( "%s failed", cmd );
+ return;
+ }
+ cfile->eval_line = 0;
+}
+
+void
+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 ) );
+ }
+}
+
+char *
+read_cline( conffile_t *cfile )
+{
+ char *ret;
+
+ ret = cfile->buf;
+ if (cfile->eval_fp) {
+ ret = fgets( cfile->buf, cfile->bufl, cfile->eval_fp );
+ cfile->eval_line++;
+ if (!ret) {
+ eval_pclose( cfile );
+ ret = cfile->buf;
+ }
+ }
+ if (!cfile->eval_fp && ret) {
+ ret = fgets( cfile->buf, cfile->bufl, cfile->fp );
+ cfile->line++;
+ }
+ return ret;
+}
+
int
getcline( conffile_t *cfile )
{
@@ -336,14 +398,24 @@ getcline( conffile_t *cfile )
conf_error( cfile, "excess token '%s'\n", arg );
cfile->err = 1;
}
- while (fgets( cfile->buf, cfile->bufl, cfile->fp )) {
- cfile->line++;
+ while (read_cline( cfile )) {
cfile->rest = cfile->buf;
if (!(cfile->cmd = get_arg( cfile, ARG_OPTIONAL, ))) {
if (comment)
