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.
bye,
Sumit