On Tue, 2014-01-07 at 20:20 +0100, Stef Walter wrote:
On 07.01.2014 19:55, Simo Sorce wrote:
> A few notes:
>
> 1. style
> although we allow things like
> if (condition)
> statement;
> I do not think we accpet for or while loops w/o braces
Nod.
> 2. the functions are incomplete and lack documentation that states so
> in particular the only accepted format type is 's' (string).
> I think we want a complete replacement for [a|sn]printf if you want to
> put it in ding-libs.
> If you do not want to make the general puprpose version then I propose
> you contribute this only as a private function for sssd, but please add
> documentation in the header file about what formatting is supported.
The reason that the printf positional arguments are so brittle (one must
consume them all) is exactly because arguments of different sizes are
accepted in the varargs. So obviously the positional code cannot know
the relevant vararg if some are not consumed.
Having the only accepted format type be 's' follows from that. Otherwise
it would be difficult to have a user-provided format string.
But yes, general purpose is not the goal. Fixing sssd to be robust and
correct is the goal. I'd much rather not put this sorta thing in general
purpose library like ding-libs.
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 ?
> 3. can you change from 'callback' to 'copy_fn'
?
Sure.
> On the code, looks mostly ok except the incompleteness of the printf
> function, however:
>
> On Tue, 2014-01-07 at 16:07 +0100, Stef Walter wrote:
>> +static void
>> +snprintf_callback (void *data,
>> + const char *piece,
>> + size_t length)
>> +{
>> + struct sprintf_ctx *cx = data;
>> +
>> + /* Don't copy if too much data */
>> + if (cx->length > cx->alloc)
>> + length = 0;
>> + else if (cx->length + length > cx->alloc)
>> + length = cx->alloc - cx->length;
>> + else
>> + length = length;
>
> The last one branch is unnecessary and a bit useless :-)
Oops. Refactoring strikes again...
:-)
Simo.
--
Simo Sorce * Red Hat, Inc * New York