Re: [SSSD] [PATCH] sss_override: support import and export
On Thu, Aug 20, 2015 at 10:39:06PM +0200, Jakub Hrozek wrote:
> On Thu, Aug 20, 2015 at 01:55:45PM +0200, Pavel Březina wrote:
> > On 08/20/2015 12:54 PM, Pavel Březina wrote:
> > >On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
> > >>On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
> > >>>On 08/18/2015 05:24 PM, Pavel Březina wrote:
> > On 08/16/2015 05:59 PM, Pavel Březina wrote:
> > >https://fedorahosted.org/sssd/ticket/2737
> > >
> > >Hi, it should work... :-) However, I wanted to make import as
> > >transaction so no changes are made if some error occurs, but I had
> > >some
> > >troubles with it so I gave up eventually.
> > >
> > >Of course, it would be quite difficult to make it safe across domains
> > >thus my intention was only to ensure that either all changes are
> > >done in
> > >a domain or none. But still leaving the possibility that changes are
> > >commited in one domain but cancelled in another.
> > >
> > >I tried to start sysdb transaction on all used domains and then
> > >commit/cancel it, writing some neat mechanism for it. However, I
> > >occasionally run into a problem when data provider hangs when
> > >trying to
> > >safe a user. It looked like some sort of race condition.
> > >
> > >Unfortunately I managed to delete the code so I can't show it to
> > >you, I
> > >think it would be a nice feature so if anyone familiar with ldb
> > >want to
> > >step in, he's welcome.
> >
> > New patches attached.
> > >>>
> > >>>I split it into more patches.
> > >>>
> > >>
> > >>There is a strange ordering between this patch and the FQDN -- these
> > >>patches can't be compiled without the FQDN one, but the FQDN doesn't
> > >>apply on master.
> > >
> > >Thanks, I fixed that. I'm sending all the patches here including the fqn
> > >one in correct order.
> > >
> > >>There are still some Coverity errors:
> > >>Error: COMPILER_WARNING:
> > >>sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
> > >>'sss_colondb_open'
> > >>sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may
> > >>be used uninitialized in this function [-Wmaybe-uninitialized]
> > >># if (fp == NULL && filename != NULL) {
> > >>#^
> > >>sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was
> > >>declared here
> > >># FILE *fp;
> > >># ^
> > >># 271| }
> > >># 272|
> > >># 273|-> if (fp == NULL && filename != NULL) {
> > >># 274| ret = errno;
> > >># 275| DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s
> > >>[%d]: %s\n",
> > >>
> > >>Error: NEGATIVE_RETURNS (CWE-394):
> > >>sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
> > >>"sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a
> > >>negative number.
> > >>sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant:
> > >>Explicitly returning negative value "-22".
> > >>sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning:
> > >>unsigned variable "fqlen" = "sss_fqname".
> > >>sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned
> > >>variable "fqlen" is incremented, which might cause an integer overflow.
> > >># 340| fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
> > >># 341| if (fqlen > 0) {
> > >># 342|-> fqlen++; /* \0 */
> > >># 343| } else {
> > >># 344| return NULL;
> > >
> > >Hopefully fixed now.
> > >
> > >>
> > >>> From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
> > >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> > >>>Date: Sat, 15 Aug 2015 13:53:25 +0200
> > >>>Subject: [PATCH 1/5] sss_override: print input name if unable to
> > >>>parse it
> > >>
> > >>ACK
> > >>
> > >>
> > >>> From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
> > >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> > >>>Date: Wed, 19 Aug 2015 12:34:08 +0200
> > >>>Subject: [PATCH 2/5] TOOLS: add sss_colondb API
> > >>>
> > >>>To simplify import/export users and groups.
> > >>
> > >>More or less ack :-) see some questions inline
> > >>
> > >>>+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
> > >>>+ struct sss_colondb *db,
> > >>>+ struct sss_colondb_read_field *table)
> > >>>+{
> > >>>+int readchars;
> > >>>+size_t linelen = 0;
> > >>>+char *line = NULL;
> > >>>+char *tcline;
> > >>>+char *rest;
> > >>>+errno_t ret;
> > >>>+int i;
> > >>>+
> > >>>+if (db->mode != SSS_COLONDB_READ) {
> > >>>+return ERR_INTERNAL;
> > >>>+}
> > >>>+
> > >>>+readchars = getline(&line, &linelen, db->file);
> > >>>+if (readchars == -1) {
> > >>>+/* Nothing was read. */
> > >>>+if (errno != 0) {
> > >>>+ret = errno;
> > >>>+DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
> > >>>+ ret, sss
Re: [SSSD] [PATCH] sss_override: support import and export
On Thu, Aug 20, 2015 at 01:55:45PM +0200, Pavel Březina wrote:
> On 08/20/2015 12:54 PM, Pavel Březina wrote:
> >On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
> >>On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
> >>>On 08/18/2015 05:24 PM, Pavel Březina wrote:
> On 08/16/2015 05:59 PM, Pavel Březina wrote:
> >https://fedorahosted.org/sssd/ticket/2737
> >
> >Hi, it should work... :-) However, I wanted to make import as
> >transaction so no changes are made if some error occurs, but I had
> >some
> >troubles with it so I gave up eventually.
> >
> >Of course, it would be quite difficult to make it safe across domains
> >thus my intention was only to ensure that either all changes are
> >done in
> >a domain or none. But still leaving the possibility that changes are
> >commited in one domain but cancelled in another.
> >
> >I tried to start sysdb transaction on all used domains and then
> >commit/cancel it, writing some neat mechanism for it. However, I
> >occasionally run into a problem when data provider hangs when
> >trying to
> >safe a user. It looked like some sort of race condition.
> >
> >Unfortunately I managed to delete the code so I can't show it to
> >you, I
> >think it would be a nice feature so if anyone familiar with ldb
> >want to
> >step in, he's welcome.
>
> New patches attached.
> >>>
> >>>I split it into more patches.
> >>>
> >>
> >>There is a strange ordering between this patch and the FQDN -- these
> >>patches can't be compiled without the FQDN one, but the FQDN doesn't
> >>apply on master.
> >
> >Thanks, I fixed that. I'm sending all the patches here including the fqn
> >one in correct order.
> >
> >>There are still some Coverity errors:
> >>Error: COMPILER_WARNING:
> >>sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
> >>'sss_colondb_open'
> >>sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may
> >>be used uninitialized in this function [-Wmaybe-uninitialized]
> >># if (fp == NULL && filename != NULL) {
> >>#^
> >>sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was
> >>declared here
> >># FILE *fp;
> >># ^
> >># 271| }
> >># 272|
> >># 273|-> if (fp == NULL && filename != NULL) {
> >># 274| ret = errno;
> >># 275| DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s
> >>[%d]: %s\n",
> >>
> >>Error: NEGATIVE_RETURNS (CWE-394):
> >>sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
> >>"sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a
> >>negative number.
> >>sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant:
> >>Explicitly returning negative value "-22".
> >>sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning:
> >>unsigned variable "fqlen" = "sss_fqname".
> >>sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned
> >>variable "fqlen" is incremented, which might cause an integer overflow.
> >># 340| fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
> >># 341| if (fqlen > 0) {
> >># 342|-> fqlen++; /* \0 */
> >># 343| } else {
> >># 344| return NULL;
> >
> >Hopefully fixed now.
> >
> >>
> >>> From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
> >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> >>>Date: Sat, 15 Aug 2015 13:53:25 +0200
> >>>Subject: [PATCH 1/5] sss_override: print input name if unable to
> >>>parse it
> >>
> >>ACK
> >>
> >>
> >>> From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
> >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> >>>Date: Wed, 19 Aug 2015 12:34:08 +0200
> >>>Subject: [PATCH 2/5] TOOLS: add sss_colondb API
> >>>
> >>>To simplify import/export users and groups.
> >>
> >>More or less ack :-) see some questions inline
> >>
> >>>+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
> >>>+ struct sss_colondb *db,
> >>>+ struct sss_colondb_read_field *table)
> >>>+{
> >>>+int readchars;
> >>>+size_t linelen = 0;
> >>>+char *line = NULL;
> >>>+char *tcline;
> >>>+char *rest;
> >>>+errno_t ret;
> >>>+int i;
> >>>+
> >>>+if (db->mode != SSS_COLONDB_READ) {
> >>>+return ERR_INTERNAL;
> >>>+}
> >>>+
> >>>+readchars = getline(&line, &linelen, db->file);
> >>>+if (readchars == -1) {
> >>>+/* Nothing was read. */
> >>>+if (errno != 0) {
> >>>+ret = errno;
> >>>+DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
> >>>+ ret, sss_strerror(ret));
> >>>+return ret;
> >>>+}
> >>>+
> >>>+return EOF;
> >>>+}
> >>>+
> >>>+/* Copy line to mem_ctx. */
> >>>+tcline = talloc_strdup(mem_ctx, line);
> >>
> >>Do we need mem_ctx in this function? What about using db (or a special
> >>memctx
> >>instead o
Re: [SSSD] [PATCH] sss_override: support import and export
On 08/20/2015 12:54 PM, Pavel Březina wrote:
On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
On 08/18/2015 05:24 PM, Pavel Březina wrote:
On 08/16/2015 05:59 PM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2737
Hi, it should work... :-) However, I wanted to make import as
transaction so no changes are made if some error occurs, but I had
some
troubles with it so I gave up eventually.
Of course, it would be quite difficult to make it safe across domains
thus my intention was only to ensure that either all changes are
done in
a domain or none. But still leaving the possibility that changes are
commited in one domain but cancelled in another.
I tried to start sysdb transaction on all used domains and then
commit/cancel it, writing some neat mechanism for it. However, I
occasionally run into a problem when data provider hangs when
trying to
safe a user. It looked like some sort of race condition.
Unfortunately I managed to delete the code so I can't show it to
you, I
think it would be a nice feature so if anyone familiar with ldb
want to
step in, he's welcome.
New patches attached.
I split it into more patches.
There is a strange ordering between this patch and the FQDN -- these
patches can't be compiled without the FQDN one, but the FQDN doesn't
apply on master.
Thanks, I fixed that. I'm sending all the patches here including the fqn
one in correct order.
There are still some Coverity errors:
Error: COMPILER_WARNING:
sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
'sss_colondb_open'
sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may
be used uninitialized in this function [-Wmaybe-uninitialized]
# if (fp == NULL && filename != NULL) {
#^
sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was
declared here
# FILE *fp;
# ^
# 271| }
# 272|
# 273|-> if (fp == NULL && filename != NULL) {
# 274| ret = errno;
# 275| DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s
[%d]: %s\n",
Error: NEGATIVE_RETURNS (CWE-394):
sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
"sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a
negative number.
sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant:
Explicitly returning negative value "-22".
sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning:
unsigned variable "fqlen" = "sss_fqname".
sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned
variable "fqlen" is incremented, which might cause an integer overflow.
# 340| fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
# 341| if (fqlen > 0) {
# 342|-> fqlen++; /* \0 */
# 343| } else {
# 344| return NULL;
Hopefully fixed now.
From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
Date: Sat, 15 Aug 2015 13:53:25 +0200
Subject: [PATCH 1/5] sss_override: print input name if unable to
parse it
ACK
From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
Date: Wed, 19 Aug 2015 12:34:08 +0200
Subject: [PATCH 2/5] TOOLS: add sss_colondb API
To simplify import/export users and groups.
More or less ack :-) see some questions inline
+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
+ struct sss_colondb *db,
+ struct sss_colondb_read_field *table)
+{
+int readchars;
+size_t linelen = 0;
+char *line = NULL;
+char *tcline;
+char *rest;
+errno_t ret;
+int i;
+
+if (db->mode != SSS_COLONDB_READ) {
+return ERR_INTERNAL;
+}
+
+readchars = getline(&line, &linelen, db->file);
+if (readchars == -1) {
+/* Nothing was read. */
+if (errno != 0) {
+ret = errno;
+DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
+ ret, sss_strerror(ret));
+return ret;
+}
+
+return EOF;
+}
+
+/* Copy line to mem_ctx. */
+tcline = talloc_strdup(mem_ctx, line);
Do we need mem_ctx in this function? What about using db (or a special
memctx
instead of db) instead, then the caller could just talloc_free_children
(or we can talloc_free_children even in this function..)
Yes, it is needed in my opinion. Let us see if we both understand the
purpose here, if not I'll add a comment.
We read line with getline() which returns a pointer that needs to be
freed. But we read data into read_field table and then use it in the
caller. The problem we need to solve is how to push line pointer to the
caller so it can be freed when data is no longer needed. Is it clear?
I can think of three solutions:
1) Pass *line as output parameter so the caller can free it.
2) Use talloc and talloc_strdup all strings to mem_ctx.
3) Use talloc but strdup t
Re: [SSSD] [PATCH] TESTS: ldap_id_cleanup timeouts
On 08/19/2015 08:26 PM, Michal Židek wrote: Hi! This is another patch to avoid failing tests in the CI (make-check-valgrind). This time the ldap_id_cleanup tests. Looks like the one second cache timeout was too short and the tests sometimes failed because they expected the entries to be still valid for a short while after they were added to sysdb. I saw the failures only on Fedora 20 CI machine. See the attached patch. Michal Hi, I just run your patch on my F22 VM and I see some trouble here... see attachment. Petr PS: I used clean GIT and your patch, nothing else. I know that this problem is another then you solved. But it is still issue. (Thu Aug 20 07:40:49:577840 2015) [sssd] [test_multidom_suite_cleanup] (0x0020): Could not delete the test config ldb file [20]: (Not a directory) (Thu Aug 20 07:40:49:577887 2015) [sssd] [test_multidom_suite_cleanup] (0x0020): Could not delete the test domain ldb file [20]: (Not a directory) (Thu Aug 20 07:40:49:577899 2015) [sssd] [test_multidom_suite_cleanup] (0x0020): Could not delete the test dir (20) (Not a directory) [==] Running 1 test(s). [ RUN ] test_id_cleanup_exp_group (Thu Aug 20 07:40:49:578773 2015) [sssd] [ldb] (0x0020): Unable to open tdb 'test_ldap_id_cleanup/tests_conf.ldb': Not a directory (Thu Aug 20 07:40:49:578786 2015) [sssd] [ldb] (0x0020): Failed to connect to 'test_ldap_id_cleanup/tests_conf.ldb' with backend 'tdb': Unable to open tdb 'test_ldap_id_cleanup/tests_conf.ldb': Not a directory (Thu Aug 20 07:40:49:578791 2015) [sssd] [confdb_init] (0x0010): Unable to open config database [test_ldap_id_cleanup/tests_conf.ldb] Could not run the test - check test fixtures [ ERROR ] test_id_cleanup_exp_group [==] 1 test(s) run. [ PASSED ] 0 test(s). FAIL test_ldap_id_cleanup (exit status: 1) ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] sss_override: support import and export
On 08/20/2015 01:31 PM, Jakub Hrozek wrote:
On Thu, Aug 20, 2015 at 12:54:47PM +0200, Pavel Březina wrote:
On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
On 08/18/2015 05:24 PM, Pavel Březina wrote:
On 08/16/2015 05:59 PM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2737
Hi, it should work... :-) However, I wanted to make import as
transaction so no changes are made if some error occurs, but I had some
troubles with it so I gave up eventually.
Of course, it would be quite difficult to make it safe across domains
thus my intention was only to ensure that either all changes are done in
a domain or none. But still leaving the possibility that changes are
commited in one domain but cancelled in another.
I tried to start sysdb transaction on all used domains and then
commit/cancel it, writing some neat mechanism for it. However, I
occasionally run into a problem when data provider hangs when trying to
safe a user. It looked like some sort of race condition.
Unfortunately I managed to delete the code so I can't show it to you, I
think it would be a nice feature so if anyone familiar with ldb want to
step in, he's welcome.
New patches attached.
I split it into more patches.
There is a strange ordering between this patch and the FQDN -- these
patches can't be compiled without the FQDN one, but the FQDN doesn't
apply on master.
Thanks, I fixed that. I'm sending all the patches here including the fqn one
in correct order.
There are still some Coverity errors:
Error: COMPILER_WARNING:
sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
'sss_colondb_open'
sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may be used
uninitialized in this function [-Wmaybe-uninitialized]
# if (fp == NULL && filename != NULL) {
#^
sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was declared here
# FILE *fp;
# ^
# 271| }
# 272|
# 273|-> if (fp == NULL && filename != NULL) {
# 274| ret = errno;
# 275| DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s [%d]:
%s\n",
Error: NEGATIVE_RETURNS (CWE-394):
sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
"sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a negative number.
sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant: Explicitly returning
negative value "-22".
sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning: unsigned variable
"fqlen" = "sss_fqname".
sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned variable
"fqlen" is incremented, which might cause an integer overflow.
# 340| fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
# 341| if (fqlen > 0) {
# 342|-> fqlen++; /* \0 */
# 343| } else {
# 344| return NULL;
Hopefully fixed now.
From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
Date: Sat, 15 Aug 2015 13:53:25 +0200
Subject: [PATCH 1/5] sss_override: print input name if unable to parse it
ACK
From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
Date: Wed, 19 Aug 2015 12:34:08 +0200
Subject: [PATCH 2/5] TOOLS: add sss_colondb API
To simplify import/export users and groups.
More or less ack :-) see some questions inline
+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
+ struct sss_colondb *db,
+ struct sss_colondb_read_field *table)
+{
+int readchars;
+size_t linelen = 0;
+char *line = NULL;
+char *tcline;
+char *rest;
+errno_t ret;
+int i;
+
+if (db->mode != SSS_COLONDB_READ) {
+return ERR_INTERNAL;
+}
+
+readchars = getline(&line, &linelen, db->file);
+if (readchars == -1) {
+/* Nothing was read. */
+if (errno != 0) {
+ret = errno;
+DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
+ ret, sss_strerror(ret));
+return ret;
+}
+
+return EOF;
+}
+
+/* Copy line to mem_ctx. */
+tcline = talloc_strdup(mem_ctx, line);
Do we need mem_ctx in this function? What about using db (or a special memctx
instead of db) instead, then the caller could just talloc_free_children
(or we can talloc_free_children even in this function..)
Yes, it is needed in my opinion. Let us see if we both understand the
purpose here, if not I'll add a comment.
We read line with getline() which returns a pointer that needs to be freed.
But we read data into read_field table and then use it in the caller. The
problem we need to solve is how to push line pointer to the caller so it can
be freed when data is no longer needed. Is it clear?
I can think of three solutions:
1) Pass *line as output parameter so the caller can free it.
2) Use tallo
Re: [SSSD] [PATCH] sss_override: support import and export
On Thu, Aug 20, 2015 at 12:54:47PM +0200, Pavel Březina wrote:
> On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
> >On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
> >>On 08/18/2015 05:24 PM, Pavel Březina wrote:
> >>>On 08/16/2015 05:59 PM, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/2737
>
> Hi, it should work... :-) However, I wanted to make import as
> transaction so no changes are made if some error occurs, but I had some
> troubles with it so I gave up eventually.
>
> Of course, it would be quite difficult to make it safe across domains
> thus my intention was only to ensure that either all changes are done in
> a domain or none. But still leaving the possibility that changes are
> commited in one domain but cancelled in another.
>
> I tried to start sysdb transaction on all used domains and then
> commit/cancel it, writing some neat mechanism for it. However, I
> occasionally run into a problem when data provider hangs when trying to
> safe a user. It looked like some sort of race condition.
>
> Unfortunately I managed to delete the code so I can't show it to you, I
> think it would be a nice feature so if anyone familiar with ldb want to
> step in, he's welcome.
> >>>
> >>>New patches attached.
> >>
> >>I split it into more patches.
> >>
> >
> >There is a strange ordering between this patch and the FQDN -- these
> >patches can't be compiled without the FQDN one, but the FQDN doesn't
> >apply on master.
>
> Thanks, I fixed that. I'm sending all the patches here including the fqn one
> in correct order.
>
> >There are still some Coverity errors:
> >Error: COMPILER_WARNING:
> >sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
> >'sss_colondb_open'
> >sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may be used
> >uninitialized in this function [-Wmaybe-uninitialized]
> ># if (fp == NULL && filename != NULL) {
> >#^
> >sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was declared
> >here
> ># FILE *fp;
> ># ^
> ># 271| }
> ># 272|
> ># 273|-> if (fp == NULL && filename != NULL) {
> ># 274| ret = errno;
> ># 275| DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s [%d]:
> >%s\n",
> >
> >Error: NEGATIVE_RETURNS (CWE-394):
> >sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
> >"sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a negative
> >number.
> >sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant:
> >Explicitly returning negative value "-22".
> >sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning: unsigned
> >variable "fqlen" = "sss_fqname".
> >sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned variable
> >"fqlen" is incremented, which might cause an integer overflow.
> ># 340| fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
> ># 341| if (fqlen > 0) {
> ># 342|-> fqlen++; /* \0 */
> ># 343| } else {
> ># 344| return NULL;
>
> Hopefully fixed now.
>
> >
> >> From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> >>Date: Sat, 15 Aug 2015 13:53:25 +0200
> >>Subject: [PATCH 1/5] sss_override: print input name if unable to parse it
> >
> >ACK
> >
> >
> >> From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> >>Date: Wed, 19 Aug 2015 12:34:08 +0200
> >>Subject: [PATCH 2/5] TOOLS: add sss_colondb API
> >>
> >>To simplify import/export users and groups.
> >
> >More or less ack :-) see some questions inline
> >
> >>+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
> >>+ struct sss_colondb *db,
> >>+ struct sss_colondb_read_field *table)
> >>+{
> >>+int readchars;
> >>+size_t linelen = 0;
> >>+char *line = NULL;
> >>+char *tcline;
> >>+char *rest;
> >>+errno_t ret;
> >>+int i;
> >>+
> >>+if (db->mode != SSS_COLONDB_READ) {
> >>+return ERR_INTERNAL;
> >>+}
> >>+
> >>+readchars = getline(&line, &linelen, db->file);
> >>+if (readchars == -1) {
> >>+/* Nothing was read. */
> >>+if (errno != 0) {
> >>+ret = errno;
> >>+DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
> >>+ ret, sss_strerror(ret));
> >>+return ret;
> >>+}
> >>+
> >>+return EOF;
> >>+}
> >>+
> >>+/* Copy line to mem_ctx. */
> >>+tcline = talloc_strdup(mem_ctx, line);
> >
> >Do we need mem_ctx in this function? What about using db (or a special memctx
> >instead of db) instead, then the caller could just talloc_free_children
> >(or we can talloc_free_children even in this function..)
>
> Yes, it is needed in my opinion. Let us see if we both understand the
> purpose
Re: [SSSD] [PATCH] sss_override: support domains that requires fqname
On 08/19/2015 11:07 PM, Jakub Hrozek wrote: On Wed, Aug 19, 2015 at 01:52:10PM +0200, Pavel Březina wrote: https://fedorahosted.org/sssd/ticket/2757 I wrote it on top of my previous override patches, but I think it will apply on master. ACK (but please fix the ordering issue with the other patches) This patch is deprecated by the one in import/export thread. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] sss_override: support import and export
On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
On 08/18/2015 05:24 PM, Pavel Březina wrote:
On 08/16/2015 05:59 PM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2737
Hi, it should work... :-) However, I wanted to make import as
transaction so no changes are made if some error occurs, but I had some
troubles with it so I gave up eventually.
Of course, it would be quite difficult to make it safe across domains
thus my intention was only to ensure that either all changes are done in
a domain or none. But still leaving the possibility that changes are
commited in one domain but cancelled in another.
I tried to start sysdb transaction on all used domains and then
commit/cancel it, writing some neat mechanism for it. However, I
occasionally run into a problem when data provider hangs when trying to
safe a user. It looked like some sort of race condition.
Unfortunately I managed to delete the code so I can't show it to you, I
think it would be a nice feature so if anyone familiar with ldb want to
step in, he's welcome.
New patches attached.
I split it into more patches.
There is a strange ordering between this patch and the FQDN -- these
patches can't be compiled without the FQDN one, but the FQDN doesn't
apply on master.
Thanks, I fixed that. I'm sending all the patches here including the fqn
one in correct order.
There are still some Coverity errors:
Error: COMPILER_WARNING:
sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
'sss_colondb_open'
sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may be used
uninitialized in this function [-Wmaybe-uninitialized]
# if (fp == NULL && filename != NULL) {
#^
sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was declared here
# FILE *fp;
# ^
# 271| }
# 272|
# 273|-> if (fp == NULL && filename != NULL) {
# 274| ret = errno;
# 275| DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s [%d]:
%s\n",
Error: NEGATIVE_RETURNS (CWE-394):
sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function
"sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a negative number.
sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant: Explicitly returning
negative value "-22".
sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning: unsigned variable
"fqlen" = "sss_fqname".
sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned variable
"fqlen" is incremented, which might cause an integer overflow.
# 340| fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
# 341| if (fqlen > 0) {
# 342|-> fqlen++; /* \0 */
# 343| } else {
# 344| return NULL;
Hopefully fixed now.
From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
Date: Sat, 15 Aug 2015 13:53:25 +0200
Subject: [PATCH 1/5] sss_override: print input name if unable to parse it
ACK
From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
Date: Wed, 19 Aug 2015 12:34:08 +0200
Subject: [PATCH 2/5] TOOLS: add sss_colondb API
To simplify import/export users and groups.
More or less ack :-) see some questions inline
+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
+ struct sss_colondb *db,
+ struct sss_colondb_read_field *table)
+{
+int readchars;
+size_t linelen = 0;
+char *line = NULL;
+char *tcline;
+char *rest;
+errno_t ret;
+int i;
+
+if (db->mode != SSS_COLONDB_READ) {
+return ERR_INTERNAL;
+}
+
+readchars = getline(&line, &linelen, db->file);
+if (readchars == -1) {
+/* Nothing was read. */
+if (errno != 0) {
+ret = errno;
+DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
+ ret, sss_strerror(ret));
+return ret;
+}
+
+return EOF;
+}
+
+/* Copy line to mem_ctx. */
+tcline = talloc_strdup(mem_ctx, line);
Do we need mem_ctx in this function? What about using db (or a special memctx
instead of db) instead, then the caller could just talloc_free_children
(or we can talloc_free_children even in this function..)
Yes, it is needed in my opinion. Let us see if we both understand the
purpose here, if not I'll add a comment.
We read line with getline() which returns a pointer that needs to be
freed. But we read data into read_field table and then use it in the
caller. The problem we need to solve is how to push line pointer to the
caller so it can be freed when data is no longer needed. Is it clear?
I can think of three solutions:
1) Pass *line as output parameter so the caller can free it.
2) Use talloc and talloc_strdup all strings to mem_ctx.
3) Use talloc but strdup the whole line to mem_ctx.
I ch
