https://fedorahosted.org/sssd/ticket/1156
Also, debug level macros used in modified functions.
One comment and one nitpick.
Comment: polease use --patience flag to git format-patch so that the patches are more readable.
Nitpick:
On Tue, 2012-08-07 at 15:26 +0200, Ondrej Kos wrote:
&num, &gids, limit, &ret);- switch (status) {
- case NSS_STATUS_TRYAGAIN:
- while ( (status = ctx->ops.initgroups_dyn(pwd->pw_name,
pwd->pw_gid,
&num_gids, &num, &gids, limit,&ret))
== NSS_STATUS_TRYAGAIN) { /* buffer too small ? */
here please use a do/whiel loop to make code more readable:
do { status = ctx->ops.initgrou ...
} while (status == NSS_STATUS_TRYAGAIN);
Simo.
Strange, I've created the patch with this alias, so patience flag should be on alias gfp='git format-patch -M -C --patience --full-index'
As for the while -> do-while, if I'd do that, then it would need another condition if (status == NSS_STATUS_TRYAGAIN) for the block of code which adjusts buffer size on lines 1250-1263. It seemed a bit pointless when you can use just one condition in while. So, should i leave it this way, or really use do-while with additional if?
Ondrej
On 08/07/2012 03:58 PM, Simo Sorce wrote:
One comment and one nitpick.
Comment: polease use --patience flag to git format-patch so that the patches are more readable.
Nitpick:
On Tue, 2012-08-07 at 15:26 +0200, Ondrej Kos wrote:
&num, &gids, limit, &ret);- switch (status) {
- case NSS_STATUS_TRYAGAIN:
- while ( (status = ctx->ops.initgroups_dyn(pwd->pw_name,
pwd->pw_gid,
&num_gids, &num, &gids, limit,&ret))
== NSS_STATUS_TRYAGAIN) { /* buffer too small ? */here please use a do/whiel loop to make code more readable:
do { status = ctx->ops.initgrou ...
} while (status == NSS_STATUS_TRYAGAIN);
Simo.
On Wed, Aug 08, 2012 at 08:56:59AM +0200, Ondrej Kos wrote:
Strange, I've created the patch with this alias, so patience flag should be on alias gfp='git format-patch -M -C --patience --full-index'
As for the while -> do-while, if I'd do that, then it would need another condition if (status == NSS_STATUS_TRYAGAIN) for the block of code which adjusts buffer size on lines 1250-1263. It seemed a bit pointless when you can use just one condition in while. So, should i leave it this way, or really use do-while with additional if?
Ondrej
The patch cannot be compiled, probably after a misplaced "fmt" reformatting which broke multi-line DEBUG messages.
+FMT fix
On 08/08/2012 10:53 AM, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 08:56:59AM +0200, Ondrej Kos wrote:
Strange, I've created the patch with this alias, so patience flag should be on alias gfp='git format-patch -M -C --patience --full-index'
As for the while -> do-while, if I'd do that, then it would need another condition if (status == NSS_STATUS_TRYAGAIN) for the block of code which adjusts buffer size on lines 1250-1263. It seemed a bit pointless when you can use just one condition in while. So, should i leave it this way, or really use do-while with additional if?
Ondrej
The patch cannot be compiled, probably after a misplaced "fmt" reformatting which broke multi-line DEBUG messages. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, 2012-08-08 at 08:56 +0200, Ondrej Kos wrote:
Strange, I've created the patch with this alias, so patience flag should be on alias gfp='git format-patch -M -C --patience --full-index'
As for the while -> do-while, if I'd do that, then it would need another condition if (status == NSS_STATUS_TRYAGAIN) for the block of code which adjusts buffer size on lines 1250-1263. It seemed a bit pointless when you can use just one condition in while. So, should i leave it this way, or really use do-while with additional if?
Well in general I prefer to have assignment of a variable as return of a function and testing it's value as separate lines as that makes it much easier to read but also makes it easier to step through in a debugger.
So as long as setting and testing status are separate I am fine with any solution really.
Simo.
Ondrej
On 08/07/2012 03:58 PM, Simo Sorce wrote:
One comment and one nitpick.
Comment: polease use --patience flag to git format-patch so that the patches are more readable.
Nitpick:
On Tue, 2012-08-07 at 15:26 +0200, Ondrej Kos wrote:
&num, &gids, limit, &ret);- switch (status) {
- case NSS_STATUS_TRYAGAIN:
- while ( (status = ctx->ops.initgroups_dyn(pwd->pw_name,
pwd->pw_gid,
&num_gids, &num, &gids, limit,&ret))
== NSS_STATUS_TRYAGAIN) { /* buffer too small ? */here please use a do/whiel loop to make code more readable:
do { status = ctx->ops.initgrou ...
} while (status == NSS_STATUS_TRYAGAIN);
Simo.
On 08/08/2012 01:53 PM, Simo Sorce wrote:
On Wed, 2012-08-08 at 08:56 +0200, Ondrej Kos wrote:
Strange, I've created the patch with this alias, so patience flag should be on alias gfp='git format-patch -M -C --patience --full-index'
As for the while -> do-while, if I'd do that, then it would need another condition if (status == NSS_STATUS_TRYAGAIN) for the block of code which adjusts buffer size on lines 1250-1263. It seemed a bit pointless when you can use just one condition in while. So, should i leave it this way, or really use do-while with additional if?
Well in general I prefer to have assignment of a variable as return of a function and testing it's value as separate lines as that makes it much easier to read but also makes it easier to step through in a debugger.
So as long as setting and testing status are separate I am fine with any solution really.
Simo.
Fixed.
Ondrej
Ondrej
On 08/07/2012 03:58 PM, Simo Sorce wrote:
One comment and one nitpick.
Comment: polease use --patience flag to git format-patch so that the patches are more readable.
Nitpick:
On Tue, 2012-08-07 at 15:26 +0200, Ondrej Kos wrote:
&num, &gids, limit, &ret);- switch (status) {
- case NSS_STATUS_TRYAGAIN:
- while ( (status = ctx->ops.initgroups_dyn(pwd->pw_name,
pwd->pw_gid,
&num_gids, &num, &gids, limit,&ret))
== NSS_STATUS_TRYAGAIN) { /* buffer too small ? */here please use a do/whiel loop to make code more readable:
do { status = ctx->ops.initgrou ...
} while (status == NSS_STATUS_TRYAGAIN);
Simo.
Ack from me, however if you do not like the if statement inside the loop you can also change it to be:
while (true) { status = .... if (status != NSS_STATUS_TRYAGAIN) { break; } .... }
Also looking at this loop I see a potential for infinite loop in case something goes wrong and the internal call keeps returning NSS_STATUS_TRYAGAIN. If we want to address that issue, we may want to change the condition to exit the loop to be a time based condition, something like:
enter_time = time(NULL);
while (enter_time + timeout < time(NULL) { status = .... if (status != NSS_STATUS_TRYAGAIN) { break; } .... }
if status == NSS_STATUS_TRYAGAIN -> error
I would like to see this (maybe in a separate patch on top of your current patch, as the proxy provider is synchronous already so not allowing it to loop for too long is a necessary condition to avoid having the monitor kill it.
But I would like to know what other think about this. A very slow nss module may end up having it's call killed midair ...
Simo.
On Wed, Aug 08, 2012 at 09:24:50AM -0400, Simo Sorce wrote:
Ack from me, however if you do not like the if statement inside the loop you can also change it to be:
while (true) { status = .... if (status != NSS_STATUS_TRYAGAIN) { break; } .... }
Ondrej, please let me know whether you're going to make this change or not. I don't really mind personally, both forms are fine from my point of view.
The patch itself works good, but unfortunately when I was testing it I found that the proxy provider is broken (and was even before your patch):
https://fedorahosted.org/sssd/ticket/1466 https://fedorahosted.org/sssd/ticket/1467
Also looking at this loop I see a potential for infinite loop in case something goes wrong and the internal call keeps returning NSS_STATUS_TRYAGAIN. If we want to address that issue, we may want to change the condition to exit the loop to be a time based condition, something like:
enter_time = time(NULL);
while (enter_time + timeout < time(NULL) { status = .... if (status != NSS_STATUS_TRYAGAIN) { break; } .... }
if status == NSS_STATUS_TRYAGAIN -> error
I would like to see this (maybe in a separate patch on top of your current patch, as the proxy provider is synchronous already so not allowing it to loop for too long is a necessary condition to avoid having the monitor kill it.
But I would like to know what other think about this. A very slow nss module may end up having it's call killed midair ...
In general this is true for any operation. Marko was hitting a bug with an extremely large AD deployment where saving data to sysdb took so long the back end got killed..
But I tend to agree, as far as I can tell, this is the only place in the SSSD where we perform both network IO and disk IO synchronously in a loop.
Care to open a ticket?
On 08/08/2012 06:10 PM, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 09:24:50AM -0400, Simo Sorce wrote:
Ack from me, however if you do not like the if statement inside the loop you can also change it to be:
while (true) { status = .... if (status != NSS_STATUS_TRYAGAIN) { break; } .... }
Ondrej, please let me know whether you're going to make this change or not. I don't really mind personally, both forms are fine from my point of view.
I'd leave it this way.
The patch itself works good, but unfortunately when I was testing it I found that the proxy provider is broken (and was even before your patch):
https://fedorahosted.org/sssd/ticket/1466 https://fedorahosted.org/sssd/ticket/1467
Also looking at this loop I see a potential for infinite loop in case something goes wrong and the internal call keeps returning NSS_STATUS_TRYAGAIN. If we want to address that issue, we may want to change the condition to exit the loop to be a time based condition, something like:
enter_time = time(NULL);
while (enter_time + timeout < time(NULL) { status = .... if (status != NSS_STATUS_TRYAGAIN) { break; } .... }
if status == NSS_STATUS_TRYAGAIN -> error
I would like to see this (maybe in a separate patch on top of your current patch, as the proxy provider is synchronous already so not allowing it to loop for too long is a necessary condition to avoid having the monitor kill it.
But I would like to know what other think about this. A very slow nss module may end up having it's call killed midair ...
In general this is true for any operation. Marko was hitting a bug with an extremely large AD deployment where saving data to sysdb took so long the back end got killed..
But I tend to agree, as far as I can tell, this is the only place in the SSSD where we perform both network IO and disk IO synchronously in a loop.
Care to open a ticket? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Aug 09, 2012 at 11:11:56AM +0200, Ondrej Kos wrote:
On 08/08/2012 06:10 PM, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 09:24:50AM -0400, Simo Sorce wrote:
Ack from me, however if you do not like the if statement inside the loop you can also change it to be:
while (true) { status = .... if (status != NSS_STATUS_TRYAGAIN) { break; } .... }
Ondrej, please let me know whether you're going to make this change or not. I don't really mind personally, both forms are fine from my point of view.
I'd leave it this way.
Fair enough, ack from me, too and pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Ondrej Kos -
Simo Sorce