On Fri, Oct 30, 2009 at 10:51:13PM +0100, Sumit Bose wrote:
On Fri, Oct 30, 2009 at 05:42:10PM -0400, Simo Sorce wrote:
> 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.
>
ok, it is now fatal as all other errors
bye,
Sumit
This new version adds a missing return after a tevent_req_done() call.
bye,
Sumit