[patch] ldb_msg_add_string()

[simo]

On Mon, 2006-09-25 at 13:18 +1000, tridge at samba.org wrote:
> Simo,
> 
>  > No, I don't think we should allow this as it may conceal a programming
>  > error in setting the attribute.
> 
> hmm, I don't really understand this. Does this imply that
> 
>  ldb_msg_add_string(ldb, "comment", "");

well actually I am more concerned about things like:

   ldb_msg_add_empty(ldb, "comment", LDB_FLAG_MOD_REPLACE);

and then forgetting to really set the new value.

> is a programming error, whereas
> 
>  ldb_msg_add_string(ldb, "comment", "foo");
> 
> isn't ?
> 
> I could understand that NULL in that place is a programming error, but
> I can't see how "" is an error.

See the ldb_msg_add_empty() example.

>  > I'm just being defensive here, but I don't think the gain we have
>  > through this is worth the possible loss.
> 
> well, I'm pretty sure that right now we have real bugs due to
> this. Just look at all the calls to samdb_msg_add_string(), which
> calls ldb_msg_add_string(). I'd be surprised to find that every one of
> those calls copes with the case where the string is zero length.

I see what you mean.

> In C APIs, the usual convention is that there is a strong separation
> of NULL strings from non-NULL strings, but no big distinction between
> strings of length zero and strings of length 1. 

That's true.

> As an alternative, we could have a:
> 
>   ldb_msg_add_directory_string()
> 
> call, which gives an error on empty strings as that isn't within the
> allowable syntax of "directory string" in ldap, but I think making our
> default "set a C string" function refuse strings because they have
> length zero is something that will bite us time and again :)

I have to think about this, maybe we can silently discard an empty
attribute but give out a level 0 debug error.
That might be a good enough compromise.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer
email: idra at samba.org
http://samba.org