[SSSD] [PATCH] add sysdb_delete_recursive request to sysdb API

Simo Sorce ssorce at redhat.com
Fri Oct 30 21:42:10 UTC 2009


On Fri, 2009-10-30 at 12:01 +0100, Sumit Bose wrote:
> On Thu, Oct 29, 2009 at 11:26:39PM +0100, Sumit Bose wrote:
> > On Thu, Oct 29, 2009 at 09:32:34PM +0000, Simo Sorce wrote:
> > > On Thu, 2009-10-29 at 19:40 +0100, Sumit Bose wrote:
> > > > On Thu, Oct 29, 2009 at 01:39:21PM +0100, Sumit Bose wrote:
> > > > > Hi,
> > > > > 
> > > > > this patch adds a recursive delete request to the sysdb API. It has
> > > > the
> > > > > same interface as sysdb_delete_entry, but does not delete the entry,
> > > > but
> > > > > its children.
> > > > > 
> > > > > bye,
> > > > > Sumit
> > > > 
> > > > This is a new version of the patch which tries to delete the entry AND
> > > > all its children. It searches all objects with a subtree search, sorts
> > > > the result so that the ones with the most components come first and
> > > > finally loops over the results and deletes them.
> > > 
> > > Comments inline.
> > > 
> > > > +
> > > > +    subreq = sysdb_search_entry_send(state, ev, handle, dn,
> > > > LDB_SCOPE_SUBTREE,
> > > > +                                     "distinguishedName=*", NULL);
> > > 
> > > Please use "(objectclass=*)" as filter to catch all entries.
> > > 
> > 
> > I would prefer to stay with distinguishedName, because it is
> > auto-generated and always present.
> > 
> > > Also please set attrs. Passing NULL, means you will retrieve all
> > > attributes wasting a lot of memory unnecessarily. You are interested
> > > only in the entries msg->dn, so you probably do not want any attribute
> > > returned at all.
> > 
> > ah, I thought NULL means nothing, now I pass { NULL }
> > 
> > > 
> > > [..]
> > > 
> > > > +static int compare_ldb_dn_comp_num(const void *m1, const void *m2)
> > > > +{
> > > > +    struct ldb_message *msg1 = talloc_get_type(*(const void **) m1,
> > > > +                                               struct ldb_message);
> > > > +    struct ldb_message *msg2 = talloc_get_type(*(const void **) m2,
> > > > +                                               struct ldb_message);
> > > > +
> > > > +    return ldb_dn_get_comp_num(msg2->dn) -
> > > > ldb_dn_get_comp_num(msg1->dn);
> > > > +}
> > > 
> > > Please move this function in sysdb.c, it's a generic function that can
> > > be used by multiple functions and here just interrupts reading the
> > > program flow.
> > 
> > done
> > 
> > > 
> > > > +static void sysdb_delete_recursive_loop(struct tevent_req *subreq)
> > > [...]
> > > 
> > > I think you should split the this function into a function that receives
> > > the results of sysdb_search_entry_recv() and then another one that sets
> > > the loop. If necessary use the trick I used in sdap_cli_connect to do
> > > continuation functions (see the sdap_cli_*_step functions).
> > > 
> > 
> > done
> > 
> > > The rest looks good to me.
> > > 
> > 
> > Thanks for reviewing.
> > 
> > bye,
> > Sumit
> > 
> 
> sorry, this new patch fixes a compiler warning.

Looks good to me, I have only a minor nitpick, shouldn't the ENOENT
error in sysdb_delete_recursive_op_done() be fatal ?

Given it should never happen, does it make sense to allow to continue ?

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list