Re: [PATCH 11/11] syslog: Use a printf buffer directly to construct the entire packet

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 15/02/24 10:02, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 09/02/24 12:26, Florian Weimer wrote:
>>> This defers buffer management largely to the asprintf implementation.
>>> It is quite close to the original implementation around
>>> open_memstream, except that an on-stack buffer is used for shorter
>>> messages, and that strftime no longer writes directly into the
>>> buffer.
>>>
>>> The new version no longer uses the (slow) %n format specifier.
>>> It also fixes an issue in the localtime_r failure path, where
>>> the message is prefixed with ": " due to an incorrect placement
>>> of the %n specifier.
>>
>> What I am not sure if this is really the direction we want for
>> internal FILE usage.  I had the impression that the printf buffer
>> internal API was mainly meant to improve the old FILE implementation
>> and its historical drawnbacks and limitations.  For internal usage we
>> would continue to use standard FILE API, should we move to always use
>> printf buffers instead?
> 
> There are currently no internal uses (that I can see) of _IO_strfile for
> writing purposes.  The psiginfo function uses __fmemopen, but that seems
> excessive.  The syslog implementation was the only user of
> open_memstream.  The reason for the open_memstream removal was
> allocation removal, which makes sense for a logging function.
> 
> We must have manual constructs that use some printf function variant to
> write to temporary buffers elsewhere.  One example is
> stdio-common/psignal.c.  I can't find others outside sunrpc/ right now,
> but they likely exist.  Switching those to fmemopen/open_memstream would
> introduce the allocation issue as well, and dynamically sized printf
> buffers could be a replacement.

For unbounded messages I tend to agree that fmemopen/open_memstream might
be not the best option.  I would like to avoid having a another internal
API for such cases, but it seems that printf buffer is a slight better
option indeed.

> 
> I know that musl uses its _IO_strfile equivalent in such cases, but our
> libio implementation is much more heavyweight, and it's hard to review
> if new direct low-level uses of the libio internals are actually
> correct.
> 
>> I am asking because it is a lot of code and refactoring for a specific
>> code that I would like to avoid change due the recent issues.  Most of 
>> complication is the static buffer optimization, so maybe we should just
>> remove it?
> 
> Pretty much all buffer management code is deleted.  We now have even
> fewer code of that than in the previous open_memstream-based function.
> 
>> Also, since the motivation for this change is just to remove the %n
>> requirement, maybe we can just not enable it on syslog instead (since
>> we now that the internal calls should not act as a gadget)?  
> 
> That doesn't remove the complicated buffer management.

Right, the resulting code does seems simplified.