On Thu, Jan 09, 2014 at 09:32:00PM +0100, Stef Walter wrote:
On 08.01.2014 23:27, Jakub Hrozek wrote:
> On Wed, Jan 08, 2014 at 09:02:52PM +0100, Stef Walter wrote:
>> On 08.01.2014 17:59, Simo Sorce wrote:
>>> On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote:
>>>> On 07.01.2014 22:21, Simo Sorce wrote:
>>>>> 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.
>>
>> I figured that neither neither realmd or sssd was using the asprintf
>> variant (both used other allocation schemes) I might as well remove it.
>
> I agree it's better to not have code that's not used in the repo at all.
> Such code can easily become subject to bitrot (it wouldn't be the
> case with a utility function like this one perhaps, but still..)
One more fix to these patches. I missed a couple signed compare problem
problems in patch 0002. fq_len and nlen are now int.
Stef
The patches look good to me and all the testing I did (various formats
of FQDN including invalid, no FQDN, groups with mixed-domain membership)
went fine.
ACK.