LDB: Patch for fixing counter variables
Matthias Dieter Wallnöfer
mdw at samba.org
Tue Nov 10 04:35:42 MST 2009
> 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.
More information about the samba-technical