On 29/04/2019 15:44, Vadim Zeitlin wrote:
On Sat, 27 Apr 2019 22:58:23 +0100 Harald van Dijk <harald@xxxxxxxxxxx> wrote:
HvD> > HvD> As a proof of concept that should clearly not be committed in its
HvD> > HvD> current form, please see the attached patch.
HvD> >
HvD> > Thanks, this is indeed much simpler than what I had in mind and I can
HvD> > confirm that it does work.
HvD> >
HvD> > What prevents this patch from being applied in the current form exactly?
HvD>
HvD> I removed a #ifdef FLUSHERR which suggests to me this is supposed to be
HvD> optional, not unconditional.
I'm not sure what the original intention was here (and "git blame" doesn't
help as these lines date from the "Initial import" commit), but flushing an
unbuffered stream doesn't make sense anyhow, so it didn't matter whether it
was optional or unconditional before. If the stream becomes buffered, we
just have to flush it now, however. I.e. I don't see how could the patch be
possibly improved in this area.
It's like with the #ifdef notyet and #ifdef USE_GLIBC_STDIO bits. It's
incomplete and doesn't work, but it's kept around nonetheless. There are
two reasons why it might be kept around: either because nobody cares
enough to remove it, or to account for the possibility that a future
change re-enables it (where "it" in this case is enabling buffering for
errout). I am betting on the latter, but it is certainly possible I am
wrong about that.
HvD> It wastes memory: the code should be re-worked so that output and
HvD> preverrout share the same buffer, given that there can never be pending
HvD> output for one when the other is being written to.
I'm a bit worried about this one, it doesn't seem completely obvious to me
that the 2 objects can't be used at the same time.
It's indeed not completely obvious, but all bits that print to output
will flush the buffer when they're done, before a next command starts
executing, before the next command requires an xtrace string to be
printed. It's usually the flushall() in evalbltin() that handles this.
So sharing the buffer
between them seems like a rather dangerous thing to do and it's certainly
not going to be pretty neither ("struct output" is clearly supposed to have
its own buffer and not use a shared one, as written currently). Isn't it an
overkill to do all this just to save BUFSIZ bytes _only_ when xtrace option
is used (so probably not affecting 99.99% of shell invocations)?
That's a fair point.
Anyhow, I'd still like to see this patch applied to dash (and hopefully
appearing in the next Debian version), but I'm not sure if I can do
anything to help with this. Please let me know if you think I can, thanks,
VZ
If you like, regardless of what happens here, you can see if Debian are
willing to add this as a custom patch to their version of dash. They do
apply some patches that are not in the official dash version.
Cheers,
Harald van Dijk
