On Mon, 22 Jun 2020 at 21:21, Willy Tarreau <w...@1wt.eu> wrote: > > Hi guys, > > On Mon, Jun 22, 2020 at 07:49:34PM +0200, Lukas Tribus wrote: > > Hello Tim, > > > > On Mon, 22 Jun 2020 at 18:56, Tim Düsterhus <t...@bastelstu.be> wrote: > > > > > > Lukas, > > > > > > Am 22.06.20 um 18:41 schrieb Lukas Tribus: > > > > On Mon, 22 Jun 2020 at 18:16, Tim Duesterhus <t...@bastelstu.be> wrote: > > > >> > > > >> Fix parsing of configurations if the configuration file does not end > > > >> with > > > >> an LF. > > > > > > > > ... but it's also warning about it at the same time. > > > > > > > > So it's unclear to me: > > > > > > > > Do we support a configuration without trailing LF or not? > > > > > > > > If yes, there is no point in a warning (just as < 2.1). > > > > If no, we should abort and error out. > > > > > > > > We can "just warn" if we plan to deprecate it in future release, and > > > > at that point, explain that fact. But I find it strange to warn about > > > > something, without a clear indication about *WHY* (unsupported, > > > > deprecated, etc). > > > > > > > > > > > > Thoughts? > > > > > > > I consider leaving out a trailing newline a bug for these reasons: > > > > > > [...] > > > A non-terminated line thus is not a line and handling non-terminated > > > lines is a bit wonky. > > > > What you are explaining is that the behavior is basically undefined, > > so in my opinion we should just flat out reject this configuration. > > s/ha_warning/ha_alert ? > > > > If we want to continue with a warning only (to not break older > > configs), let's elaborate, because the warning just explains that the > > line is not terminated, but not if and why that is actually a problem, > > which I don't like to leave as an exercise for the reader (user). > > > > ha_warning("parsing [%s:%d]: line is not terminated by a newline (LF / > > '\\n'), this may lead to undefined behavior.\n", > > > > ha_warning("parsing [%s:%d]: line should be terminated by a newline > > (LF / '\\n'), otherwise undefined behavior may occur.\n", > > > > Something like that? > > > > > > But really, I think we should just reject this instead (then the text > > suffices, because it actually stops working). > > It used to work and certainly happens from time to time to people who > generate their own configs. It's too easy to concatenate pieces of > strings and not have the final condition ready to emit the last LF. > For example if you emit you cookie options based on various tests, > you may end up appending words without the trailing "\n" and decide > to emit one at the beginning of each line instead. I've seen such > configs from time to time so I know they do happen. > > I think I could be fine with dropping support for this (unless somebody > objects here), but not in the LTS branch and not without a prior warning, > so that users have time to fix their scripts. > > Also if we change this we have to update the doc to mention that all > lines must be terminated. > > It's not uncommon to see files missing the last LF, and Git even has a > warning for this. But the main issue to be reported there is that the > file might have accidently been truncated (e.g. file-system full during > the copy). However I agree with you Lukas that the warning should > clearly indicate the impact and what needs to be fixed. > > But it gets tricky. There's a known flaw of fgets() making it return an > incomplete line: if a \0 is present on the line before the \n. And given > that fgets returns a pointer to a zero-terminated string instead of a > length, the only way to read the string is to read it up to \0. So a > line not ending with \n is not necessarily a truncated one but might be > one with a \0 anywhere in the middle. > > So what we could do if we want to do something clean is the following: > - detect \0 or truncation on a line and note its position ; > - if we find another line once a truncated line has been found, we > emit a warning about "stray NUL character at position X on line Y, > this will not be supported in 2.3 anymore" and reset this position. > - at the end of the loop, if the last NUL position is still set, we > emit a warning about "missing LF on last line, file might have been > truncated, this will not be supported in 2.3 anymore". > > And in 2.3 we turn that into errors. > > What do you guys think about it ?
I like it, yes. Lukas