On Fri, Aug 26, 2016 at 10:01:56AM +0200, Fabiano Fidêncio wrote: > On Fri, Aug 26, 2016 at 9:49 AM, Lukas Slebodnik <lsleb...@redhat.com> wrote: > > On (26/08/16 08:10), Fabiano Fidêncio wrote: > >>Jakub, > >> > >>On Thu, Aug 25, 2016 at 1:56 PM, Jakub Hrozek <jhro...@redhat.com> wrote: > >>> On Wed, Aug 17, 2016 at 03:13:57PM +0200, Fabiano Fidêncio wrote: > >>>> The attached patch for #3125 [0]is based on Jakub's "secrets" branch > >>>> on github [1], as there are (at least) a few issues with the current > >>>> secrets code. > >>>> > >>>> [0]: https://fedorahosted.org/sssd/ticket/312 > >>>> [1]: https://github.com/jhrozek/sssd/tree/secrets > >>>> > >>>> Although I didn't fire a CI build, the local "make intgcheck" passes > >>>> without issues. > >>>> > >>>> Best Regards, > >>>> -- > >>>> Fabiano Fidêncio > >>> > >>> Sorry about the late reply. The patch works fine as I would expect: > >>> > >>> sudo curl -H "Content-Type: application/json" --unix-socket > >>> /var/run/secrets.socket -XDELETE http://localhost/secrets/xxx > >>> <html> > >>> <head> > >>> <title>404 Not Found</title></head> > >>> <body> > >>> <h1>Not Found</h1> > >>> <p>The requested resource was not found.</p> > >>> </body> > >>> > >>> Nonetheless, I think the patch can be a bit simpler: > >>> > >>>> From bc39f7ba0ffa75a9ca972266bbc2741cbb0c8c73 Mon Sep 17 00:00:00 2001 > >>>> From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> > >>>> Date: Wed, 17 Aug 2016 13:12:21 +0200 > >>>> Subject: [PATCH] SECRETS: Check whether a secret exists before trying to > >>>> delete it > >>>> MIME-Version: 1.0 > >>>> Content-Type: text/plain; charset=UTF-8 > >>>> Content-Transfer-Encoding: 8bit > >>>> > >>>> Checking whether a secret exists or not before trying to delete it > >>>> allows SSSD to report the proper error code in case of trying to delete > >>>> a non-existent secret. > >>>> > >>>> Resolves: > >>>> https://fedorahosted.org/sssd/ticket/3125 > >>>> > >>>> Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> > >>>> --- > >>>> src/responder/secrets/local.c | 14 ++++++++++++++ > >>>> 1 file changed, 14 insertions(+) > >>>> > >>>> diff --git a/src/responder/secrets/local.c > >>>> b/src/responder/secrets/local.c > >>>> index e91766f..3985b6d 100644 > >>>> --- a/src/responder/secrets/local.c > >>>> +++ b/src/responder/secrets/local.c > >>>> @@ -376,12 +376,26 @@ int local_db_delete(TALLOC_CTX *mem_ctx, > >>>> struct local_context *lctx, > >>>> const char *req_path) > >>>> { > >>>> + static const char *attrs[] = { "secret", NULL }; > >>>> struct ldb_dn *dn; > >>>> + struct ldb_result *res; > >>>> int ret; > >>>> > >>>> ret = local_db_dn(mem_ctx, lctx->ldb, req_path, &dn); > >>>> if (ret != EOK) goto done; > >>>> > >>>> + ret = ldb_search(lctx->ldb, mem_ctx, &res, dn, LDB_SCOPE_BASE, > >>>> + attrs, "%s", LOCAL_SIMPLE_FILTER); > >>>> + if (ret != EOK) { > >>>> + ret = ENOENT; > >>>> + goto done; > >>>> + } > >>>> + > >>>> + if (res->count == 0) { > >>>> + ret = ENOENT; > >>>> + goto done; > >>>> + } > >>>> + > >>>> ret = ldb_delete(lctx->ldb, dn); > >>> > >>> I think it's OK to just try to delete the secret and act on the return > >>> value. This is what we already have in sysdb_delete_cache_entry(): > >>> 105 ret = ldb_delete(ldb, dn); > >>> 106 switch (ret) { > >>> 107 case LDB_SUCCESS: > >>> 108 return EOK; > >>> 109 case LDB_ERR_NO_SUCH_OBJECT: > >>> 110 if (ignore_not_found) { > >>> 111 return EOK; > >>> 112 } > >>> > >>> So I think we can use something else in secrets to return ENOENT on > >>> receiving LDB_ERR_NO_SUCH_OBJECT from ldb_delete(). > >> > >>Suggestion taken. You can find the v2 attached to this email. > >> > >>Best Regards, > >>-- > >>Fabiano Fidêncio > > > > >From 3e71f901ab12f5e5c570dcf0796ef47e379e7a53 Mon Sep 17 00:00:00 2001 > >>From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> > >>Date: Wed, 17 Aug 2016 13:12:21 +0200 > >>Subject: [PATCH v2] SECRETS: Return ENOENT when_deleting a non-existent > >>secret > >>MIME-Version: 1.0 > >>Content-Type: text/plain; charset=UTF-8 > >>Content-Transfer-Encoding: 8bit > >> > >>The approach taken is quite similar to what is done by > >>sysdb_delete_cache_entry(). > >> > >>Resolves: > >>https://fedorahosted.org/sssd/ticket/3125 > >> > >>Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> > >>--- > >> src/responder/secrets/local.c | 14 ++++++++------ > >> 1 file changed, 8 insertions(+), 6 deletions(-) > >> > >>diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c > >>index e91766f..c32af8c 100644 > >>--- a/src/responder/secrets/local.c > >>+++ b/src/responder/secrets/local.c > >>@@ -380,15 +380,17 @@ int local_db_delete(TALLOC_CTX *mem_ctx, > >> int ret; > >> > >> ret = local_db_dn(mem_ctx, lctx->ldb, req_path, &dn); > >>- if (ret != EOK) goto done; > >>+ if (ret != EOK) return ret; > >> > >> ret = ldb_delete(lctx->ldb, dn); > >>- if (ret != EOK) { > >>- ret = EIO; > >>+ switch (ret) { > >>+ case LDB_SUCCESS: > >>+ return EOK; > >>+ case LDB_ERR_NO_SUCH_OBJECT: > >>+ return ENOENT; > >>+ default: > >>+ return EIO; > >> } > > Is this mapping ldb_error to ernno different as in sysdb_error_to_errno? > > If no then it would be good to reuse existing fucntion. > > Hmm. Interesting. Didn't even know about this function. > Suggestion taken, see v3 attached. > > Best Regards, > -- > Fabiano Fidêncio
Thanks, ACK (still works, code looks good) _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
