Re: (ITS#4627) back-ldif issues

[hyc@symas.com]

h.b.furuseth@usit.uio.no wrote:
> Pierangelo Masarati writes:
>>> Error handling:
>>>
>>> ldif_tool_entry_first() cannot return NOID.
>> Dunno (untouched)
> 
> Howard fixed.  One remaining issue: ldif_tool_entry_next() increments
> li->li_tool_current even when it returns NOID.  Should it do that?

Harmless, but stupid. Fixed now.
> 
>> (...)
>>> back-ldif overwrites files instead writing to a temp file and
>>> renaming when complete.  The current way corrupts entries if a
>>> write fails halfway through, e.g. due to a full disk. Also the
>>> file can be left truncated if internal slapd operations fail.
>>> If the OS supports it, back-ldif should make a new file in the
>>> same directory and rename to the old filename when complete.
>> Fixed; now mkstemp(3) is used, and rename(2) only in case of success.
> 
> Not there yet...  you create it in the current directory instead
> of in the ldif directory.
> 
> Possibly it's best to create it in the top directory of the ldif
> database.  That way, if a temp file is somehow created and not
> cleaned away, it's (a) in the easiest place to see and (b) won't
> prevent rmdir() if one tries to delete that database or whatever.

I would use "rm -rf" to delete the DB, in which case (b) is a moot point.

> I don't know if there are non-Unix OSes which need special system calls
> to move file between directories though.  Or for that matter, if they
> allow rename() to replace an existing file, maybe unlink is needed
> first.  The code looks very Unixy already, so I assume a Unix emulation
> is needed to run it anyway.  I don't know how those work.

Yes, we require POSIX semantics here. rename() unlinks the target atomically; 
it would be a mistake to code this in a different manner.

> Also there were some spew_entry() coding errors, rev 1.70 fixes them.
> 
>> (...)
>>> .inum should be signed, or should be read with strtoul instead of
>>> strtol.
>> strtoul(3) used

I think strtoul() here is a mistake, and we've fixed/refixed this point a 
couple times already. Remember that frontendDB has an index of {-1} in the 
config tree.

>>> I suppose it and cmp in the function should be ber_int_t,
>>> since LDAP integers are typically 32-bit.
>> Not sure what you mean.
> 
> I don't remember.  Maybe the number can end up being sent over the
> protocol, and it's possible to create a filename with a long > max
> ber_int_t so the wrong number will be sent?
> 


-- 
   -- Howard Chu
   Chief Architect, Symas Corp.  http://www.symas.com
   Director, Highland Sun        http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP     http://www.openldap.org/project/