The first three patches are just refactoring. The last patch implements the real modify operation along with a number of unit tests.
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.
On Mon, 2012-12-03 at 23:34 +0100, Jakub Hrozek wrote:
From af555ccfa228ea931d1eb31cc1dedd6f6d3580ce Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@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.
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@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.
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
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.
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.
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.
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.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce