On Fri, Jul 18, 2014 at 01:15:25PM +0200, Pavel Březina wrote:
The code looks good, but needs to be rebased on top of PavelR's patches I just pushed (sorry, first comes, first served..)
On 07/21/2014 12:07 PM, Jakub Hrozek wrote:
On Fri, Jul 18, 2014 at 01:15:25PM +0200, Pavel Březina wrote:
The code looks good, but needs to be rebased on top of PavelR's patches I just pushed (sorry, first comes, first served..)
The problem is that the first patch is basically the same as Pavel wrote. It just contains a comment explaining why ENOENT error is acceptable and a little bit different debug message. Feel free to pick whatever version you think is better, rebasing is very simple.
The second patch applies correctly.
On Mon, Jul 21, 2014 at 02:10:34PM +0200, Pavel Březina wrote:
On 07/21/2014 12:07 PM, Jakub Hrozek wrote:
On Fri, Jul 18, 2014 at 01:15:25PM +0200, Pavel Březina wrote:
The code looks good, but needs to be rebased on top of PavelR's patches I just pushed (sorry, first comes, first served..)
The problem is that the first patch is basically the same as Pavel wrote. It just contains a comment explaining why ENOENT error is acceptable and a little bit different debug message. Feel free to pick whatever version you think is better, rebasing is very simple.
Ah, I didn't realize this is the same place as well.
The second patch applies correctly.
OK, I will review it.
On Tue, Jul 22, 2014 at 09:33:01AM +0200, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 02:10:34PM +0200, Pavel Březina wrote:
On 07/21/2014 12:07 PM, Jakub Hrozek wrote:
On Fri, Jul 18, 2014 at 01:15:25PM +0200, Pavel Březina wrote:
The code looks good, but needs to be rebased on top of PavelR's patches I just pushed (sorry, first comes, first served..)
The problem is that the first patch is basically the same as Pavel wrote. It just contains a comment explaining why ENOENT error is acceptable and a little bit different debug message. Feel free to pick whatever version you think is better, rebasing is very simple.
Ah, I didn't realize this is the same place as well.
The second patch applies correctly.
OK, I will review it.
0001: I kinda like the comment, feel free to re-send with just the comment
0002: Can you drop state->num_missing_sids? Appears to be unused, the array is NULL-terminated anyway
On 07/22/2014 10:43 AM, Jakub Hrozek wrote:
On Tue, Jul 22, 2014 at 09:33:01AM +0200, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 02:10:34PM +0200, Pavel Březina wrote:
On 07/21/2014 12:07 PM, Jakub Hrozek wrote:
On Fri, Jul 18, 2014 at 01:15:25PM +0200, Pavel Březina wrote:
The code looks good, but needs to be rebased on top of PavelR's patches I just pushed (sorry, first comes, first served..)
The problem is that the first patch is basically the same as Pavel wrote. It just contains a comment explaining why ENOENT error is acceptable and a little bit different debug message. Feel free to pick whatever version you think is better, rebasing is very simple.
Ah, I didn't realize this is the same place as well.
The second patch applies correctly.
OK, I will review it.
0001: I kinda like the comment, feel free to re-send with just the comment
Done.
0002: Can you drop state->num_missing_sids? Appears to be unused, the array is NULL-terminated anyway
It is used. There is a little bit of confusion because some functions returns NULL terminated arrays others not so we have to keep both. Looking into the code I think it should be possible to use only NULL terminated arrays. Shall I create a refactoring ticket?
On Tue, Jul 22, 2014 at 03:32:18PM +0200, Pavel Březina wrote:
On 07/22/2014 10:43 AM, Jakub Hrozek wrote:
On Tue, Jul 22, 2014 at 09:33:01AM +0200, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 02:10:34PM +0200, Pavel Březina wrote:
On 07/21/2014 12:07 PM, Jakub Hrozek wrote:
On Fri, Jul 18, 2014 at 01:15:25PM +0200, Pavel Březina wrote:
The code looks good, but needs to be rebased on top of PavelR's patches I just pushed (sorry, first comes, first served..)
The problem is that the first patch is basically the same as Pavel wrote. It just contains a comment explaining why ENOENT error is acceptable and a little bit different debug message. Feel free to pick whatever version you think is better, rebasing is very simple.
Ah, I didn't realize this is the same place as well.
The second patch applies correctly.
OK, I will review it.
0001: I kinda like the comment, feel free to re-send with just the comment
Done.
0002: Can you drop state->num_missing_sids? Appears to be unused, the array is NULL-terminated anyway
It is used. There is a little bit of confusion because some functions returns NULL terminated arrays others not so we have to keep both. Looking into the code I think it should be possible to use only NULL terminated arrays.
Ah, you're right, I was confused because at one spot num_missing_sids is used as input and at others as output, sorry.
ACK to both, then.
Shall I create a refactoring ticket?
You can, maybe it would be better to do the change in master right away while you still know the code?
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina