-----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----- _______________________________________________ Dev mailing list [email protected] https://lists.strongswan.org/mailman/listinfo/dev
