Add sysdb_{add,replace,delete}_string() and sysdb_{add,replace,delete}_ulong()

[Fabiano Fidêncio]

Please, see the attached patches.

Best Regards,
-- 
Fabiano Fidêncio

[Jakub Hrozek]

On Thu, May 26, 2016 at 04:18:32PM +0200, Fabiano Fidêncio wrote:
>  Please, see the attached patches. 
Hey Fabiano,

Thank you for the patches!

I admit I haven't tested the patches yet, just scrolled through the
diffs. See some comments inline. But I would also like someone else to
chime in because we talked about the code on IRC, so I'm not the right
person to review the patches on my own.

>  
>  Best Regards,
>  -- 
>  Fabiano Fidêncio 
>  From 6985f8c5ac2491829af99d71b2ecb7926a38fe26 Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
>  Date: Wed, 25 May 2016 20:16:58 +0200
>  Subject: [PATCH 1/4] sysdb: move add_string() convenience to sysdb.c 
OK, makes sense to do isolated changes.

>  From fbb071581f0b90ffe85499d46641038e8b55ccaa Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
>  Date: Wed, 25 May 2016 21:12:18 +0200
>  Subject: [PATCH 2/4] sysdb: add sysdb_{add,replace,delete}_string()
>  MIME-Version: 1.0
>  Content-Type: text/plain; charset=UTF-8
>  Content-Transfer-Encoding: 8bit
>  
>  As the add_string() convenience can add, replace or delete a string
>  according to the operation received as its argument, some confusion can
>  easily happen due to its misleading name.
>  
>  In order to improve the explicitness of our code, let's introduce
>  sysdb_add_string(), sysdb_replace_string() and sysdb_delete_string().
>  
>  These new functions are basically wrappers of add_string() (now
>  sysdb_ldb_msg_string_helper()), calling it using the proper flag
>  according to each function.
>  
>  Any code previously using add_string() is now adapted to use these brand
>  new functions. 
I just wonder about the names here. The new functions operate on
ldb_message, so I guess a better name might be something like
sysdb_ldb_msg*.

The reason is that the 'native' type of sysdb is sysdb_attrs and without
the function name saying otherwise, I would expect the function to operate
on sysdb_attrs.

>  From f4e3d4e3e7fb51c3a61fe2014217a930ae3a0091 Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
>  Date: Wed, 25 May 2016 21:40:28 +0200
>  Subject: [PATCH 3/4] sysdb: move add_ulong() convenience to sysdb.c 
Makes sense to move code before changes in an isolated change.

>  From daaf63d4e378611a2fbba541dafe9dedae7934eb Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
>  Date: Wed, 25 May 2016 23:00:59 +0200
>  Subject: [PATCH 4/4] sysdb: add sysdb_{add,replace,delete}_ulong() 
Same comment about the name here as I had about patch 2/4.

[Fabiano Fidêncio]

Jakub,

On Thu, May 26, 2016 at 11:56 PM, Jakub Hrozek <jhrozek(a)redhat.com&gt; wrote:
>  On Thu, May 26, 2016 at 04:18:32PM +0200, Fabiano Fidêncio wrote:
> > Please, see the attached patches.
>
>  Hey Fabiano,
>
>  Thank you for the patches! 
Thanks for the review.

>  I admit I haven't tested the patches yet, just scrolled through the
>  diffs. See some comments inline. But I would also like someone else to
>  chime in because we talked about the code on IRC, so I'm not the right
>  person to review the patches on my own. 
Okay, makes sense.

> >
> > Best Regards,
> > --
> > Fabiano Fidêncio
>
> > From 6985f8c5ac2491829af99d71b2ecb7926a38fe26 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
> > Date: Wed, 25 May 2016 20:16:58 +0200
> > Subject: [PATCH 1/4] sysdb: move add_string() convenience to sysdb.c
>
>  OK, makes sense to do isolated changes.
>
> > From fbb071581f0b90ffe85499d46641038e8b55ccaa Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
> > Date: Wed, 25 May 2016 21:12:18 +0200
> > Subject: [PATCH 2/4] sysdb: add sysdb_{add,replace,delete}_string()
> > MIME-Version: 1.0
> > Content-Type: text/plain; charset=UTF-8
> > Content-Transfer-Encoding: 8bit
> >
> > As the add_string() convenience can add, replace or delete a string
> > according to the operation received as its argument, some confusion can
> > easily happen due to its misleading name.
> >
> > In order to improve the explicitness of our code, let's introduce
> > sysdb_add_string(), sysdb_replace_string() and sysdb_delete_string().
> >
> > These new functions are basically wrappers of add_string() (now
> > sysdb_ldb_msg_string_helper()), calling it using the proper flag
> > according to each function.
> >
> > Any code previously using add_string() is now adapted to use these brand
> > new functions.
>
>  I just wonder about the names here. The new functions operate on
>  ldb_message, so I guess a better name might be something like
>  sysdb_ldb_msg*.
>
>  The reason is that the 'native' type of sysdb is sysdb_attrs and without
>  the function name saying otherwise, I would expect the function to operate
>  on sysdb_attrs. 
Suggestion taken. It's fixed in the v2 (attached to this email).

> > From f4e3d4e3e7fb51c3a61fe2014217a930ae3a0091 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
> > Date: Wed, 25 May 2016 21:40:28 +0200
> > Subject: [PATCH 3/4] sysdb: move add_ulong() convenience to sysdb.c
>
>  Makes sense to move code before changes in an isolated change.
>
> > From daaf63d4e378611a2fbba541dafe9dedae7934eb Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
> > Date: Wed, 25 May 2016 23:00:59 +0200
> > Subject: [PATCH 4/4] sysdb: add sysdb_{add,replace,delete}_ulong()
>
>  Same comment about the name here as I had about patch 2/4. 
Same here, fixed in the v2 (attached to this email).

-- 
Fabiano Fidêncio

[Petr Cech]

Hi Fabiano,

thanks for patch set.

The first version passed CI tests. The second version looks good to me, 
I will finally ack your patch set after second run of CI tests.

Regards

-- 
Petr^4 Čech

[Petr Cech]

On 05/27/2016 07:19 AM, Petr Cech wrote:
>  Hi Fabiano,
>
>  thanks for patch set.
>
>  The first version passed CI tests. The second version looks good to me,
>  I will finally ack your patch set after second run of CI tests. 
Hi Fabiano,

CI tests passed the second version of patch set:
http://sssd-ci.duckdns.org/logs/job/44/01/summary.html

=> ACK

>  Regards 
-- 
Petr^4 Čech

[Jakub Hrozek]

On Fri, May 27, 2016 at 07:41:51AM +0200, Petr Cech wrote:
>  On 05/27/2016 07:19 AM, Petr Cech wrote:
>  > Hi Fabiano,
>  > 
>  > thanks for patch set.
>  > 
>  > The first version passed CI tests. The second version looks good to me,
>  > I will finally ack your patch set after second run of CI tests.
>  
>  Hi Fabiano,
>  
>  CI tests passed the second version of patch set:
>  http://sssd-ci.duckdns.org/logs/job/44/01/summary.html
>  
>  => ACK 
Pushed to master:
    * 5b1e73bc40a55f2095660423a2a4623a93de1ef8
    * a928f7a6bd7681db6e26cba3eb7da22d14288737
    * 7b9a4a89cb92a0281d73a2c2e79f5eeb317e1149
    * 49d84c926b00ba1368372cdec255bceb58d66f43