[SSSD] [PATCH] INI allow string list to have empty values

Jakub Hrozek jhrozek at redhat.com
Thu Jan 21 16:24:48 UTC 2010


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

>>> This is the last of the non ELAPI patche that I have in my repo.
>>
>>
>> The only functional problem I see is that the implementation does not
>> strip leading whitespace, i.e. " foo, bar " would become { " foo", "bar"
>> }. Otherwise seems to work fine. Some comments below:
>>
> The leading space at the beginning of the value is stripped when value
> is read from
> key = value
> line in the INI file.
> And then the logic cleans the spaces around separator so we should be
> fine here.
> 

Ah, sorry, I was only looking at the individual function and missed that.

>> Is there any reason to arbitrarily limit the number of separators?
>>
>> Also, maybe it would be nicer to use strpbrk() instead of
>> for(j = 0; j < lensep; j++) {
>> if(buff[i] == locsep[j])
>> ..one less loop.
>>
> 
> I do not see a reason why we should not limit them.
> strpbrk definitely has a loop inside so this is a wash.
> I think that limited number of separators reminds developers not go
> "wild" with the format.
> IMO there should be one separator. 3 is a courtesy.
> The down side of using strpbrk() is that it is not known which separator
> actually matched.
> Though this capability is not used I had it in mind for testing purposes.
> Changing to strpbrk would cause me to rewrite the logic and retest it.
> Do you think it is worth it?
> 

If there's any functionality planned that involves the character that
matched, then no, I proposed strpbrk() mainly to get rid of the for()
and if().

>> Instead of:
>> while ((i < dlen) && (isspace(*start)))
>> maybe you could use:
>> while (*start && (isspace(*start)))
>> as the data will be null-terminated strings anyway. That way you could
>> avoid doing i++ before the loop and i-- immediatelly after it.
> I need to adjust "i" anyways to skip the spaces and not process them
> twice so I would have to save initial position and then get delta and
> then adjust "i" using delta.
> I think in general case you suggestion is correct but in this case it
> makes sense to do it as written.
> 

OK.

- -> ACK
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEUEARECAAYFAktYf9AACgkQHsardTLnvCUVRgCcCTr/2wVOUlrkzvOPe5uUGam2
ew0Al3IyS1i8Wsk1Y/okVAeFjvryr6A=
=GcKq
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list