You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@apache.org> on 2018/01/15 16:42:02 UTC
1.10 API review - svn_io_stdin_readline()
About this new-for-1.10 API:
[[[
/** Reads a string from stdin until a newline or EOF is found
*
* @since New in 1.10.
*/
svn_error_t *
svn_io_stdin_readline(const char **result,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
]]]
Compare with svn_stream_readline() and svn_io_file_readline():
* they read into a svn_stringbuf_t;
* they have EOL and EOF params;
* svn_io_file_readline() has max_len param;
It seems to me it would be better to make this new function more similar
to them.
Thoughts?
- Julian
Re: 1.10 API review - svn_io_stdin_readline()
Posted by Stefan Sperling <st...@apache.org>.
On Tue, Jan 16, 2018 at 09:36:42AM +0000, Julian Foad wrote:
> Stefan Sperling wrote:
> > On Tue, Jan 16, 2018 at 07:37:31AM +0100, Branko Čibej wrote:
> > > 2. What's wrong with using svn_stream_readline() with
> > > svn_io_stream_for_stdin2()?
> > >
> > > In other words: I suspect this new function should be removed.
> [...]
> >
> > This function is in fact just a convenience wrapper around the two other
> > functions you mention.
> >
> > There are 7 callers of this function across our command line tools.
> > If this function is not deemed appropriate for the public API, I would just
> > move it to the libsvn_subr cmdline.c bag of helper functions. Agreed?
>
> That is also the thought that came to me overnight.
>
> Renamed and moved to svn_cmdline__stdin_readline() in r1821224.
>
> Thanks.
> - Julian
Thanks Julian!
Re: 1.10 API review - svn_io_stdin_readline()
Posted by Julian Foad <ju...@apache.org>.
Stefan Sperling wrote:
> On Tue, Jan 16, 2018 at 07:37:31AM +0100, Branko Čibej wrote:
>> 2. What's wrong with using svn_stream_readline() with
>> svn_io_stream_for_stdin2()?
>>
>> In other words: I suspect this new function should be removed.
[...]
>
> This function is in fact just a convenience wrapper around the two other
> functions you mention.
>
> There are 7 callers of this function across our command line tools.
> If this function is not deemed appropriate for the public API, I would just
> move it to the libsvn_subr cmdline.c bag of helper functions. Agreed?
That is also the thought that came to me overnight.
Renamed and moved to svn_cmdline__stdin_readline() in r1821224.
Thanks.
- Julian
Re: 1.10 API review - svn_io_stdin_readline()
Posted by Stefan Sperling <st...@apache.org>.
On Tue, Jan 16, 2018 at 07:37:31AM +0100, Branko Čibej wrote:
> Two questions here:
> 1. Why did none of the existing functions in libsvn_subr/prompt.c fit
> the use case?
They assume a terminal. I'm not sure if they'd work on stdin as-is.
> 2. What's wrong with using svn_stream_readline() with
> svn_io_stream_for_stdin2()?
>
> In other words: I suspect this new function should be removed.
>
> -- Brane
Just for the record, this function was added by William Orr in this
patch submission: https://svn.haxx.se/dev/archive-2017-12/0146.shtml
Based on feedback I provided here:
https://svn.haxx.se/dev/archive-2017-09/0106.shtml
This function is in fact just a convenience wrapper around the two other
functions you mention.
There are 7 callers of this function across our command line tools.
If this function is not deemed appropriate for the public API, I would just
move it to the libsvn_subr cmdline.c bag of helper functions. Agreed?
Re: 1.10 API review - svn_io_stdin_readline()
Posted by Branko Čibej <br...@apache.org>.
On 15.01.2018 22:22, Stefan Sperling wrote:
> On Mon, Jan 15, 2018 at 04:42:02PM +0000, Julian Foad wrote:
>> About this new-for-1.10 API:
>> [[[
>> /** Reads a string from stdin until a newline or EOF is found
>> *
>> * @since New in 1.10.
>> */
>> svn_error_t *
>> svn_io_stdin_readline(const char **result,
>> apr_pool_t *result_pool,
>> apr_pool_t *scratch_pool);
>> ]]]
>>
>> Compare with svn_stream_readline() and svn_io_file_readline():
>> * they read into a svn_stringbuf_t;
>> * they have EOL and EOF params;
>> * svn_io_file_readline() has max_len param;
>>
>> It seems to me it would be better to make this new function more similar to
>> them.
>>
>> Thoughts?
>>
>> - Julian
> I wouldn't disagree with any such changes. Consistency is good!
>
> There's currently only one use case for this function (read a password
> from stdin) but these callers could easily be adapted.
Two questions here:
1. Why did none of the existing functions in libsvn_subr/prompt.c fit
the use case?
2. What's wrong with using svn_stream_readline() with
svn_io_stream_for_stdin2()?
In other words: I suspect this new function should be removed.
-- Brane
Re: 1.10 API review - svn_io_stdin_readline()
Posted by Stefan Sperling <st...@apache.org>.
On Mon, Jan 15, 2018 at 04:42:02PM +0000, Julian Foad wrote:
> About this new-for-1.10 API:
> [[[
> /** Reads a string from stdin until a newline or EOF is found
> *
> * @since New in 1.10.
> */
> svn_error_t *
> svn_io_stdin_readline(const char **result,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool);
> ]]]
>
> Compare with svn_stream_readline() and svn_io_file_readline():
> * they read into a svn_stringbuf_t;
> * they have EOL and EOF params;
> * svn_io_file_readline() has max_len param;
>
> It seems to me it would be better to make this new function more similar to
> them.
>
> Thoughts?
>
> - Julian
I wouldn't disagree with any such changes. Consistency is good!
There's currently only one use case for this function (read a password
from stdin) but these callers could easily be adapted.
Re: 1.10 API review - svn_io_stdin_readline()
Posted by Branko Čibej <br...@apache.org>.
On 16.01.2018 03:42, Nathan Hartman wrote:
>> On Jan 15, 2018, at 11:42 AM, Julian Foad <ju...@apache.org> wrote:
>>
>> About this new-for-1.10 API:
>> [[[
>> /** Reads a string from stdin until a newline or EOF is found
>> *
>> * @since New in 1.10.
>> */
>> svn_error_t *
>> svn_io_stdin_readline(const char **result,
>> apr_pool_t *result_pool,
>> apr_pool_t *scratch_pool);
>> ]]]
>>
>> Compare with svn_stream_readline() and svn_io_file_readline():
>> * they read into a svn_stringbuf_t;
>> * they have EOL and EOF params;
>> * svn_io_file_readline() has max_len param;
>>
>> It seems to me it would be better to make this new function more similar to them.
>>
>> Thoughts?
>>
>> - Julian
> I didn't look at the context but it looks like, at a bare minimum, this function needs a size of result param to avoid overrunning a buffer.
No, that's not a problem because the function allocates the buffer and
returns a pointer to it.
-- Brane
Re: 1.10 API review - svn_io_stdin_readline()
Posted by Nathan Hartman <ha...@gmail.com>.
> On Jan 15, 2018, at 11:42 AM, Julian Foad <ju...@apache.org> wrote:
>
> About this new-for-1.10 API:
> [[[
> /** Reads a string from stdin until a newline or EOF is found
> *
> * @since New in 1.10.
> */
> svn_error_t *
> svn_io_stdin_readline(const char **result,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool);
> ]]]
>
> Compare with svn_stream_readline() and svn_io_file_readline():
> * they read into a svn_stringbuf_t;
> * they have EOL and EOF params;
> * svn_io_file_readline() has max_len param;
>
> It seems to me it would be better to make this new function more similar to them.
>
> Thoughts?
>
> - Julian
I didn't look at the context but it looks like, at a bare minimum, this function needs a size of result param to avoid overrunning a buffer.