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?= pbrezina@redhat.com 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?= pbrezina@redhat.com 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:
- Pass *line as output parameter so the caller can free it.
- Use talloc and talloc_strdup all strings to mem_ctx.
- Use talloc but strdup the whole line to mem_ctx.
I chose 3) since it contains less allocations and it is quite simple and elegant.
You can't free data in sss_colondb_readline() because you don't know when the caller stops using them. And calling talloc_free_children on encapsulated db context is not safe. Yes, it is doable now since it doesn't contain any children, but the context structure can be changed any time in the future.
I would argue that when the caller calls sss_colondb_readline he's done using the data from the previous line.
In this use case, yes. This way it is more generic though.
Maybe I don't understand what you mean by "special memctx" since that is what I use there.
I meant to have something like: sss_colondb_ctx -+ +-- readline_ctx
And allocate the line contents internally on readline_ctx.
But I'm not sure if an internal interface is worth all the hassle..
Well, I stand by current code but I don't have any valid arguments to oppose you since this is doable as well :-) You're the one who's going to ack it, so it's your call.
- free(line);
- line = NULL;
From 0877b501f01dff26a312944a47a98cb88eb47d1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Wed, 19 Aug 2015 12:43:15 +0200 Subject: [PATCH 3/5] sss_override: decompose code better
ACK
thanks for splitting the patch, makes the review much easier.
From 1556d7f744302e8e7e3aa4e3459c5a4c7467bbd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Wed, 19 Aug 2015 12:35:12 +0200 Subject: [PATCH 4/5] sss_override: support import and export
Please ask Dan to add the import and export to his tests.
Please also ask him to test UPN in the first column -- that's what's the Dell tool supports.
UPNs won't work yet, I think. I need to finally recreate my AD environment before I start working on those.
Please do, do you want me to file a ticket so the effort is tracked? I haven't tested this scenario either, but the NSS responder supports lookups by UPNs for some time now...
Yes please.
I'll take a look at the patches..