[PATCH] PROXY: fix negative cache

[Ondrej Kos]

https://fedorahosted.org/sssd/ticket/1685

when trying to delete credentials not present in cache, the return value 
was misread, taking ENOENT as a failed part of code.

Fixing patch is attached

Ondra

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: [phone]
cell:  [phone]
ext:   82-62558
loc:   1013 Brno 1 office
irc:   okos @ #brno

[Jakub Hrozek]

On Fri, Dec 07, 2012 at 08:59:40PM +0100, Ondrej Kos wrote:
>  https://fedorahosted.org/sssd/ticket/1685
>  
>  when trying to delete credentials not present in cache, the return
>  value was misread, taking ENOENT as a failed part of code.
>  
>  Fixing patch is attached
>  
>  Ondra 
Looks good, just one question:

>  +static int
>  +delete_user(struct sysdb_ctx *sysdb, const char *name, uid_t uid)
>  +{
>  +    int ret = EOK;
>  +
>  +    DEBUG(SSSDBG_TRACE_FUNC,
>  +          ("User %s does not exist (or is invalid) on remote server,"
>  +           " deleting!\n", name));
>  +    ret = sysdb_delete_user(sysdb, name, uid);
>  +    if (ret == ENOENT) {
>  +        ret = EOK;
>  +        errno = EOK; 
Why set errno? We don't use it (I hope) to check the results of delete
user..

>  +    }
>  +
>  +    return ret;
>  +}
>  + 
The rest looks good. I haven't tested the patch yet.

[Ondrej Kos]

On 10/12/12 10:38, Jakub Hrozek wrote:
>  On Fri, Dec 07, 2012 at 08:59:40PM +0100, Ondrej Kos wrote:
> > https://fedorahosted.org/sssd/ticket/1685
> >
> > when trying to delete credentials not present in cache, the return
> > value was misread, taking ENOENT as a failed part of code.
> >
> > Fixing patch is attached
> >
> > Ondra
>
>  Looks good, just one question:
>
> > +static int
> > +delete_user(struct sysdb_ctx *sysdb, const char *name, uid_t uid)
> > +{
> > +    int ret = EOK;
> > +
> > +    DEBUG(SSSDBG_TRACE_FUNC,
> > +          ("User %s does not exist (or is invalid) on remote server,"
> > +           " deleting!\n", name));
> > +    ret = sysdb_delete_user(sysdb, name, uid);
> > +    if (ret == ENOENT) {
> > +        ret = EOK;
> > +        errno = EOK;
>
>  Why set errno? We don't use it (I hope) to check the results of delete
>  user.. 
Good catch, I was firstly confused by the error message produced:
[sssd[nss]] [sss_dp_get_reply] (0x1000): Got reply from Data Provider - 
DP error code: 3 errno: 2 error

so i originally for test reset both return value and errno, but looking 
at the code now, we don't use errno.

New patch is attached.

> > +    }
> > +
> > +    return ret;
> > +}
> > +
>
>  The rest looks good. I haven't tested the patch yet.
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>  

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: [phone]
cell:  [phone]
ext:   82-62558
loc:   1013 Brno 1 office
irc:   okos @ #brno

[Jakub Hrozek]

On Mon, Dec 10, 2012 at 11:00:41AM +0100, Ondrej Kos wrote:
>  On 10/12/12 10:38, Jakub Hrozek wrote:
>  >On Fri, Dec 07, 2012 at 08:59:40PM +0100, Ondrej Kos wrote:
>  >>https://fedorahosted.org/sssd/ticket/1685
>  >>
>  >>when trying to delete credentials not present in cache, the return
>  >>value was misread, taking ENOENT as a failed part of code.
>  >>
>  >>Fixing patch is attached
>  >>
>  >>Ondra
>  >
>  >Looks good, just one question:
>  >
>  >>+static int
>  >>+delete_user(struct sysdb_ctx *sysdb, const char *name, uid_t uid)
>  >>+{
>  >>+    int ret = EOK;
>  >>+
>  >>+    DEBUG(SSSDBG_TRACE_FUNC,
>  >>+          ("User %s does not exist (or is invalid) on remote server,"
>  >>+           " deleting!\n", name));
>  >>+    ret = sysdb_delete_user(sysdb, name, uid);
>  >>+    if (ret == ENOENT) {
>  >>+        ret = EOK;
>  >>+        errno = EOK;
>  >
>  >Why set errno? We don't use it (I hope) to check the results of delete
>  >user..
>  
>  Good catch, I was firstly confused by the error message produced:
>  [sssd[nss]] [sss_dp_get_reply] (0x1000): Got reply from Data
>  Provider - DP error code: 3 errno: 2 error
>  
>  so i originally for test reset both return value and errno, but
>  looking at the code now, we don't use errno.
>  
>  New patch is attached. 
Ack

[Jakub Hrozek]

On Mon, Dec 10, 2012 at 06:01:34PM +0100, Jakub Hrozek wrote:
>  On Mon, Dec 10, 2012 at 11:00:41AM +0100, Ondrej Kos wrote:
>  > On 10/12/12 10:38, Jakub Hrozek wrote:
>  > >On Fri, Dec 07, 2012 at 08:59:40PM +0100, Ondrej Kos wrote:
>  > >>https://fedorahosted.org/sssd/ticket/1685
>  > >>
>  > >>when trying to delete credentials not present in cache, the return
>  > >>value was misread, taking ENOENT as a failed part of code.
>  > >>
>  > >>Fixing patch is attached
>  > >>
>  > >>Ondra
>  > >
>  > >Looks good, just one question:
>  > >
>  > >>+static int
>  > >>+delete_user(struct sysdb_ctx *sysdb, const char *name, uid_t uid)
>  > >>+{
>  > >>+    int ret = EOK;
>  > >>+
>  > >>+    DEBUG(SSSDBG_TRACE_FUNC,
>  > >>+          ("User %s does not exist (or is invalid) on remote
> server,"
>  > >>+           " deleting!\n", name));
>  > >>+    ret = sysdb_delete_user(sysdb, name, uid);
>  > >>+    if (ret == ENOENT) {
>  > >>+        ret = EOK;
>  > >>+        errno = EOK;
>  > >
>  > >Why set errno? We don't use it (I hope) to check the results of delete
>  > >user..
>  > 
>  > Good catch, I was firstly confused by the error message produced:
>  > [sssd[nss]] [sss_dp_get_reply] (0x1000): Got reply from Data
>  > Provider - DP error code: 3 errno: 2 error
>  > 
>  > so i originally for test reset both return value and errno, but
>  > looking at the code now, we don't use errno.
>  > 
>  > New patch is attached.
>  
>  Ack 
Pushed to master and sssd-1-9