On Fri, Aug 26, 2016 at 09:49:27AM +0200, Lukas Slebodnik 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.
As long as the errno codes would not translate into some unexpected HTTP error code, I agree. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
