On 01/18/2016 11:30 PM, Michal Židek wrote: > Hi! > > see attached simple patch for ticket: > https://fedorahosted.org/sssd/ticket/2912 > > The first patch is for master and 1.13 > the second for 1.12. > > It seems like the decision to ignore cache > validity when background refresh is enabled > was not a good one and it was not possible > to fetch new netgroup entry even if the > old was invalidated with sss_cache. > > Michal Nack. The decision was good, but the patch was wrong. The point was to disable midpoint refresh if background refresh is on. The check should be moved to this area in check_cache(): if (ret == EAGAIN) { /* No callback required * This was an out-of-band update. We'll return EOK * so the calling function can return the cached entry * immediately. */ something like: if (ret == EAGAIN && is_refreshed_on_bg()) then ret = EOK (just return) Also please extend is_refreshed_on_bg() to return true for users and groups since it is already supported.

On 01/19/2016 10:41 AM, Pavel Březina wrote: > On 01/19/2016 10:37 AM, Pavel Březina wrote: >> On 01/18/2016 11:30 PM, Michal Židek wrote: >>> Hi! >>> >>> see attached simple patch for ticket: >>> https://fedorahosted.org/sssd/ticket/2912 >>> >>> The first patch is for master and 1.13 >>> the second for 1.12. >>> >>> It seems like the decision to ignore cache >>> validity when background refresh is enabled >>> was not a good one and it was not possible >>> to fetch new netgroup entry even if the >>> old was invalidated with sss_cache. >>> >>> Michal >> >> Nack. >> >> The decision was good, but the patch was wrong. The point was to disable >> midpoint refresh if background refresh is on. The check should be moved >> to this area in check_cache(): >> >> if (ret == EAGAIN) { >> /* No callback required >> * This was an out-of-band update. We'll return EOK >> * so the calling function can return the cached entry >> * immediately. >> */ >> >> something like: if (ret == EAGAIN && is_refreshed_on_bg()) then ret = >> EOK (just return) >> >> Also please extend is_refreshed_on_bg() to return true for users and >> groups since it is already supported. > > Ah, I just saw your second patch that does exactly this thing. I guess > you can ignore this nack then, I'll review both patches in more detail > later. Ack to this patch.