trusted domains n+4 and related stuff

[Rafal Szczesniak]

On Thu, Nov 21, 2002 at 07:48:48PM -0500, tridge at samba.org wrote:
> Mimir,
> 
> Thanks for the patch!
> 
> A few comments:
> 
> - in ipstr_list_add you try to be too fancy. I suspect the chunking
>   stuff is to try to second guess the malloc implementation and
>   allocate in bigger lumps? Don't do that unless there is good
>   profiling evidence to suggest that it is needed. It is too easy to
>   get wrong and just complicates the code.

hmm. ok.

> - in ipstr_list_add you do this:
> 
> 	if (ipstr)
> 		safe_strcat(ipstr, ":", sizeof(ipstr));
> 	else
> 		return NULL;
> 
>   but ipstr is a fstring, so it can never be null. Only pointers can be
>   NULL, and ipstr is an array (arrays can never be NULL in C).

Yes! Thank you for this catch. That was a legacy after the previous
version of this code (which had "char* ipstr" in it). It's good to get
someone other's pair of eyes over it...

> A simpler varient of that function would be something like this
> (untested code warning ...)
> 
> char* ipstr_list_add(char **ipstr_list, const struct in_addr *ip)
> {
> 	char *new_str = NULL;
> 
> 	if (*ipstr_list) {
> 		asprintf(&new_str, "%s:%s", *ipstr_list, inet_ntoa(*ip));
> 		free(*ipstr_list);
> 	} else {
> 		new_str = strdup(inet_ntoa(*ip));
> 	}
> 	
> 	*ipstr_list = new_str;
> 	return new_str;
> }
> 

Believe it or not, but that was my initial design :-)
The reason why I gave it up was too frequent allocation (with asprintf)
and freeing. Anyway if you think it has more advantages, then, sure,
let it be.

> and yes, I know its not terribly allocator efficient, but it has the
> big advantage of being simple. I doubt allocator efficiency matters in
> this function as the lists will typically be very short. Also note
> that I removed the redundent ipstr_size argument. As the strings are
> null terminated it isn't needed (unless you're playing allocator
> efficiency games).

I basically assumed it's better to avoid to frequent use of allocation
operations.

> Similarly, ipstr_list_make() can be made much simpler.
> 
> - I think it might be better to use ',' instead of ':' for IP list
>   separation. It doesn't matter now, but when we come to do IPv6 then
>   it might matter, as iirc IPv6 uses ':' inside the string
>   representation of addresses.

Good point. You're right.

> The parse function also tries to play allocation games.

For the same reason (look above).

> A simpler
> function might be something like this:
> 
> int ipstr_list_parse(const char* ipstr_list, struct in_addr** ip_list)
> {
> 	int count;
> 	for (ip_list=NULL, count=0; ipstr_list; count++) {
> 		struct in_addr a;
> 
> 		if (inet_aton(ipstr_list, &a) == -1) break;
> 
> 		*ip_list = Realloc(*ip_list, (count+1) * sizeof(struct in_addr));
> 		if (!ip_list) {
> 			return -1;
> 		}
> 
> 		(*ip_list)[count] = a;
> 
> 		ipstr_list = strchr(ipstr_list, ':');
> 		if (ipstr_list) ipstr_list++;
> 	}
> 	return count;
> }

Looks clear. I've modified the code and I'm starting to make a few tests.

> The rest of the patch looks good!

Nice to hear :)


-- 
cheers,
+------------------------------------------------------------+
|Rafal 'Mimir' Szczesniak <mimir at diament.ists.pwr.wroc.pl>   |
|*BSD, GNU/Linux and Samba                                  /
|__________________________________________________________/