On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote:
On 07.01.2014 22:21, Simo Sorce wrote:
> On Tue, 2014-01-07 at 21:31 +0100, Stef Walter wrote:
>> On 07.01.2014 20:34, Simo Sorce wrote:
>>> Ok fine, makes sense once explained (need this explanation in the
>>> docs/headers), but then use a different name.
>>>
>>> If I see safe_snprintf, I assume the full format capabilities of
>>> snprintf, the name in this case is misleading.
>>>
>>> That said I am not sure what's the best name you could use...
>>> maybe strextend ? and the astrextend variant for allocated ones ?
>>
>> I've called it safe_str_snprintf (and so on). Hopefully that's clear
>> enough. It is processing the familiar printf syntax, so it makes sense
>> to have that somewhere.
>
> Personally the problem is in using 'printf' in there (both in function
> name and file names), because although similar it doesn't support all of
> printf semantics, and it can't, it will confuse peole.
> I do not think a _str_ prefix really makes a difference.
> The other issue is that you may later find people "extending"
> safe_snprintf() to "support the other format arguments" and we are back
> to an "unsafe" function.
>
> Maybe: safe_format_string() ?
Sure ... done.
Thanks.
>> Updated header documentation as requested, and made other
fixes from
>> your earlier review.
>
> Sorry I forgot another, I think you should either set errno on errors,
> or return an errno_t instead of -1. Just returning -1 for all errors is
> a poor interface.
It's the same interface as the functions being replaced: snprintf and
asprintf. We return the full format length, or -1 on failure. snprintf
and friends don't set errno. See man printf(3).
But I guess now that we're diverging from the interface provided by
snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it
makes you happy.
Well I generally prefer to know if the format was wrong or ENOMEM
happened, but I see you removed the alloc version ... I guess in this
case I am not too picky, only format errors can happen now.
Simo.
--
Simo Sorce * Red Hat, Inc * New York