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( cfil
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 hand
