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?= <fiden...@redhat.com>
> 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?= <fiden...@redhat.com>
> 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?= <fiden...@redhat.com>
> 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?= <fiden...@redhat.com>
> 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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
