[strongSwan-dev] configurable SPI-range in charon

[Thomas Egerer]

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

Hello Martin,
Martin Willi schrobtete:
> Hi Thomas,
> 
> Thanks for your patch-set. Some comments:
> 
>> +       this->public.get_char = (char(*)(settings_t*, char *key, char def, ...))get_char;
>> +       this->public.get_int8 = (int8_t(*)(settings_t*, char *key, int8_t def, ...))get_int8;
>> +       this->public.get_uint8 = (u_int8_t(*)(settings_t*, char *key, u_int8_t def, ...))get_uint8;
>> +       this->public.get_int16 = (int16_t(*)(settings_t*, char *key, int16_t def, ...))get_int16;
>> +       this->public.get_uint16 = (u_int16_t(*)(settings_t*, char *key, u_int16_t def, ...))get_uint16;
>> +       this->public.get_int32 = (int(*)(settings_t*, char *key, int def, ...))get_int;
>> +       this->public.get_uint32 = (u_int32_t(*)(settings_t*, char *key, u_int32_t def, ...))get_uint;
>> +       this->public.get_uint = (u_int32_t(*)(settings_t*, char *key, u_int32_t def, ...))get_uint;
>> +       this->public.get_int64 = (int64_t(*)(settings_t*, char *key, int64_t def, ...))get_int64;
>> +       this->public.get_uint64 = (u_int64_t(*)(settings_t*, char *key, u_int64_t def, ...))get_uint64; 
> 
> Do we really need this full set of methods? I fully agree to a
> get_uint(), but the 8/16/32 getters actually just check the size of the
> resulting value. The caller could check the sanity of the values where
> appropriate, as you do it in the kernel_interface anyway at some degree.
Well, most of the getters are not required. The reason why I introduced
the get_uint32 in first place was that an SPI is an unsigned 32 bit
value and get_int uses strtol which can only deal positive values up to
0x7FFFFFFF, while strtoul as used in get_uint32 can deal with the full
range of an unsigned 32 bit value.
The get_char getter is quite useful for me, since I used it to create my
own modified rng_t which returns a string of all the same character.
Perhaps you're right about the most other getters. They might not be
required at all. But on the other hand don't range checks inside the
settings_t itself make more sense since you do the checks in *one*
single place instead of all over the place whenever and wherever you
need a certain data type.

> The 64 getters might make sense, but are they used anywhere yet?
Nope, not as far as I know.

> I'd like to keep the settings_t interface as simple as possible. I have
> some plans to modularize the settings layer (to e.g. store the values in
> a SQL database).
Sounds interesting, as long as the 'boring' old strongswan.conf can
still be used.

> Having a too complex interface makes implementation of
> such a backend more and more complex.
In my point of view adding some more getters does not necessarily make
the interface that complex. But it's your decision after all. It would
be helpful if you included at least the get_char, get_uint32 and perhaps
the get_[u]int64 (these two might be useful in the future)

Regards
Thomas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkujPcIACgkQDXd94wpQmdxQYACgqsBjhtdsHKGcu9dV0AotS8KG
axMAnRNIkIu/EXpZbxlFQI6k5ul4d44l
=HA1C
-----END PGP SIGNATURE-----