LDB: Patch for fixing counter variables

Matthias Dieter Wallnöfer mdw at samba.org
Tue Nov 10 04:35:42 MST 2009


Andrew,
> It isn't very efficient, but just like the const changes it is easy to
> slip in other changes and subtle bugs in a big 'cleanup' patch.
>
> That's why we need to be careful doing this, and why splitting things up
> helps, because it allows me or others to say '1 is ok, still thinking
> about 2'.  The more you split it up, the easier it is for me.
>    
But for me more work-intensive. Isn't it possible for you to provide me 
a list of functions/files which should be reviewed or corrected?
> Also, for me the whole question of 'is there any point' is still very
> much open.
>
> That is, the boundary condition needs to be *way* below 2^31 (can we
> really allocate that many elements?), and raising it to 2^32 just seems
> to change the rollover point.
> BTW, See: http://xkcd.com/571/
>    
Well, we can also let this changes. But then I would like to see also 
the size specifiers ("num_elements", "num_attributes"...) as signed int 
(e.g. when we would have implemented this in Java this would be the 
right solution). I only dislike this kind of mixing.
> Most of the one I've looked at seem OK, but because it's so large I've
> not been able to look at them all.  However some things look like they
> could be improved, such as the change to ldb_msg_remove_element(), which
> looks like it should be a ptrdiff_t, and check for both above and below
> the range.
>
> And this change:
>   char *ldb_binary_encode(void *mem_ctx, struct ldb_val val)
>   {
> -	int i;
>   	char *ret;
> -	int len = val.length;
> +	unsigned int i, len = val.length;
>   	unsigned char *buf = val.data;
>
> Would have been better as just a search/replace - the construction 'i,
> len = val.length' isn't as nice.
>    
Thanks for pointing this out! A change on this will happen from my side.
> I'm sorry this is so tedious, and to be so picky.  Like the const
> changes, it will take time to check all these (and 'make test' generally
> isn't sufficient on these kind of changes).
>    
But still I see this not completely unimportant. I handle this as a side 
project like the "const"s next to my actual "main project" 
("password_hash"). I try to provide a first mergeable version soon 
(which does support changes from the administrator side with password 
policies and for now lets the "samdb_set_password" call in the actual 
form to handle also user passwords right). I only need to rethink about 
the removed attribute constraints which you pointed out in another post.

Matthias


More information about the samba-technical mailing list