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