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
