10:57 a.m.
The first three patches are just refactoring. The last patch implements
the real modify operation along with a number of unit tests.
4:34 p.m.
On Tue, Nov 27, 2012 at 05:57:28PM +0100, Jakub Hrozek wrote:
The first three patches are just refactoring. The last patch
implements
the real modify operation along with a number of unit tests.
The attached patches include some minor changes and bugfixes found later
during development. The concept is the same.
5 p.m.
On Mon, 2012-12-03 at 23:34 +0100, Jakub Hrozek wrote:
--
Simo Sorce * Red Hat, Inc * New York
From af555ccfa228ea931d1eb31cc1dedd6f6d3580ce Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 26 Nov 2012 13:18:59 +0100
Subject: [PATCH 1/4] MEMBEROF: split processing the member modify into
a
separate function
- /* if we need to add something put it away so that it
- * can be done after all delete operations are over */
- if (added && added->num) {
- mod_ctx->to_add = added;
- }
-
- /* if we have something to remove do it first */
- if (removed && removed->num) {
- return mbof_mod_delete(mod_ctx, removed);
- }
-
- /* if there is nothing to remove and we have stuff to add
- * do it right away */
- if (mod_ctx->to_add) {
- return mbof_mod_add(mod_ctx, added);
- }
It seem like the above comments are lost in the new code, I would retain
them unless they do not apply anymore.
The rest looks good.
Simo.
5:06 p.m.
On Mon, 2012-12-03 at 23:34 +0100, Jakub Hrozek wrote:
From 7b963c4d6ffcfb793feb22ca9a966a3f4c9d284f Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Tue, 27 Nov 2012 16:09:23 +0100
Subject: [PATCH 4/4] MEMBEROF: Implement the modify operation for
ghost users
Code looks ok, but it definitely needs more commenting.
I see you haven't touched the big comments that explain how the memberof
plugin does calculation, thst's probably because the previous patch that
added ghost support didn't either.
I think we need a small blurb that explains how memberof is behaving wrt
ghost elements so that we can read what the code is trying to do and
correlate it with the code at hand to see that it does what is claimed.
In this case a comment is necessary because the code is too complex to
figure it out quickly just by looking at it.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
6:30 a.m.
On Mon, Dec 03, 2012 at 06:14:39PM -0500, Simo Sorce wrote:
On Mon, 2012-12-03 at 23:34 +0100, Jakub Hrozek wrote:
>
> The attached patches include some minor changes and bugfixes found
> later
> during development. The concept is the same.
>
For some reason the first patch does not apply on master ...
Simo.
The patch to implement the delete operation for ghost users must be
applied first. This is how my branch looks like:
MEMBEROF: Keep inherited ghost users around on modify operation
MEMBEROF: Implement the modify operation for ghost users
MEMBEROF: Split the add ghost operation into a separate function
MEMBEROF: Split the del ghost attribute op into a reusable function
MEMBEROF: split processing the member modify into a separate function
MEMBEROF: Implement delete operation for ghost users
6:50 a.m.
On Tue, Dec 04, 2012 at 01:30:01PM +0100, Jakub Hrozek wrote:
MEMBEROF: Implement delete operation for ghost users
For some reason this patch doesn't show up in patchwork. I'll resubmit
it with some more comments about the ghost attribute deletion, hopefully
patchwork will pick it up then.
7:31 a.m.
On Tue, 2012-12-04 at 13:50 +0100, Jakub Hrozek wrote:
On Tue, Dec 04, 2012 at 01:30:01PM +0100, Jakub Hrozek wrote:
> MEMBEROF: Implement delete operation for ghost users
For some reason this patch doesn't show up in patchwork. I'll resubmit
it with some more comments about the ghost attribute deletion, hopefully
patchwork will pick it up then.
My fault,
I marked it suprseeded in error. I'll bring it back from dead, now I get
why the rest didn't apply.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
9:39 a.m.
On Tue, 2012-12-04 at 08:31 -0500, Simo Sorce wrote:
On Tue, 2012-12-04 at 13:50 +0100, Jakub Hrozek wrote:
> On Tue, Dec 04, 2012 at 01:30:01PM +0100, Jakub Hrozek wrote:
> > MEMBEROF: Implement delete operation for ghost users
>
> For some reason this patch doesn't show up in patchwork. I'll resubmit
> it with some more comments about the ghost attribute deletion, hopefully
> patchwork will pick it up then.
My fault,
I marked it suprseeded in error. I'll bring it back from dead, now I get
why the rest didn't apply.
Sorry Jakub, even with the missing patch it seem git am fails here.
Maybe you can send a complete patchset from scratch rebased on top of
master ?
Thanks,
Simo.
--
Simo Sorce * Red Hat, Inc * New York
2:44 p.m.
On Tue, Dec 04, 2012 at 10:39:26AM -0500, Simo Sorce wrote:
On Tue, 2012-12-04 at 08:31 -0500, Simo Sorce wrote:
> On Tue, 2012-12-04 at 13:50 +0100, Jakub Hrozek wrote:
> > On Tue, Dec 04, 2012 at 01:30:01PM +0100, Jakub Hrozek wrote:
> > > MEMBEROF: Implement delete operation for ghost users
> >
> > For some reason this patch doesn't show up in patchwork. I'll resubmit
> > it with some more comments about the ghost attribute deletion, hopefully
> > patchwork will pick it up then.
>
> My fault,
> I marked it suprseeded in error. I'll bring it back from dead, now I get
> why the rest didn't apply.
Sorry Jakub, even with the missing patch it seem git am fails here.
Maybe you can send a complete patchset from scratch rebased on top of
master ?
Done.
These patches has been included in (and superseded by) the thread called:
"[PATCH] Ghost user related fixes to the memberof plugin"
in order to make the dependencies between patches easier to resolve.
3948
days inactive
3955
days old
sssd-devel@lists.fedorahosted.org
9 comments
2 participants
participants (2)
-
Jakub Hrozek -
Simo Sorce