-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 03/12/2010 10:37 AM, Dmitri Pal wrote:
Stephen Gallagher wrote:
> On 03/11/2010 12:38 PM, Dmitri Pal wrote:
>> Hi,
>
>> Patch 1 - one-liner, turn off man page generation
> Ack.
>
>> Patch 2 - cleaning INI unit test to comply with formatting guidelines
>> and to turn off endless output in case of "make check".
> Ack.
>
>> Patch 3 - addressing ticket #170
> Nack. Please update the function names to reflect that the argument they
> take is uint32_t, etc
> e.g. col_add_int_property() -> col_add_int32_property()
>
> I am aware that you will need to update any places in the collection and
> INI where these functions are called.
Yes that would have a much bigger impact. I did not have to change the
unit test
since the type conversion is happening automatically. May be I should have?
I wanted to make changes compact as much as possible.
I used the same logic as Simo mentioned - the name of the function gives
a hint.
Do you insist on the change?
If you do I will do the change.
No, I suppose it's not a real issue, since the function prototype
guarantees the type. I withdraw this concern. Jakub's concern about
strtol() is valid, however. See below for my recommendation on how to
fix this.
Also do you think I should change the unit test to use xxx32_t variables
inside tests functions instead of ints and longs?
>
>> Patch 4 - continuation of the work related on patch 3 regarding INI
>> interface that uses collection interface internally.
> Nack. (Nitpick)
> In the get_*_config_value() functions, if the return value of
> get_long_config_value() is outside the expected range, the correct error
> code to return should be ERANGE, not EIO.
>
I will reply both to you and Jakub here.
1) I agree with your comment and will change the error code.
2) As for the issue Jakub has noticed. Yes I am not 100% comfortable
with this too.
But I was trying to reduce the scope of the changes. It looks like the
right way would be
to create 4 separate functions instead wrapping one into another and use
strtoul, and strtoull.
I'd like to suggest that you follow the example given in:
http://git.fedorahosted.org/git/sssd.git?p=sssd.git;a=blob;f=src/util/str...
In the SSSD, we decided that it was easiest to just rely on strtoll()
over strtol() because it will be guaranteed to provide the largest range
available on the platform.
In the future, if we decide that the performance is insufficient, we ma
add #ifdefs based on the size of long on the system. Having these
conversions done already in a wrapper like strtouint32() will make this
optimization easier in the future.
But, in short, I suggest copying the strtonum.c code and extending it to
support all of the type sizes that we care about. It might not be a bad
idea to move that into a static common library at some point, so we
don't have to maintain it in two places, but we don't need to do that
right now.
But it is a more drastic change that I was trying to avoid.
Let me know if you think that this change is justified at the moment. If
you think it is the patch will be underway.
I think we should change this now, since we are already aware that there
is a potential bug in place.
Now should we then change the names of the functions for the INI too.
If we change the collection code names why not change both?
Yeah, that's probably the right way to go.
Please let me know how I should proceed.
Thanks for review!
- --
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAkuaiPsACgkQeiVVYja6o6PADQCeI2GFkwnndAK2XX0N7xVsGX/M
TxMAn2RbOK8LEU13+j7ZTgunIHMIsMSe
=r78v
-----END PGP SIGNATURE-----