[PATCH] GPO: Use sss_unique_file and close fd on failure

[Jakub Hrozek]

Hi,

one more usage of sss_unique_file I forgot to send earlier.

[Pavel Březina]

On 08/18/2015 03:35 PM, Jakub Hrozek wrote:
> Hi,
>
> one more usage of sss_unique_file I forgot to send earlier.

Nack.
>   ret = close(fd);
> fd = -1;
>  if (ret == -1) {
>      ret = errno;
>      DEBUG(SSSDBG_CRIT_FAILURE,
>
> @@ -369,12 +366,16 @@ static errno_t gpo_cache_store_file(const char *smb_path,
>           goto done;
>       }

Please, move this close(fd) call to done part, so we close fd only there.
> ret = EOK;
> done:
> if (ret != EOK) {
>   DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
> }

And when you are here, can you fix indentation in the debug statement above?
> if (fd != -1) {
>    close(fd);
> }
> talloc_free(tmp_ctx);
> return ret;
> }
>
> -- 2.4.3
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Wed, Aug 26, 2015 at 02:21:23PM +0200, Pavel Březina wrote:
> On 08/18/2015 03:35 PM, Jakub Hrozek wrote:
> > Hi,
> >
> > one more usage of sss_unique_file I forgot to send earlier.
>
> Nack.
> >  ret = close(fd);
> > fd = -1;
> > if (ret == -1) {
> >     ret = errno;
> >     DEBUG(SSSDBG_CRIT_FAILURE,
> >
> > @@ -369,12 +366,16 @@ static errno_t gpo_cache_store_file(const char *smb_path,
> >          goto done;
> >      }
>
> Please, move this close(fd) call to done part, so we close fd only there.
> > ret = EOK;
> > done:
> > if (ret != EOK) {
> >      DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
> >    }
>
> And when you are here, can you fix indentation in the debug statement above?
> > if (fd != -1) {
> >    close(fd);
> > }
> > talloc_free(tmp_ctx);
> >    return ret;
> >
> > }
> > -- 2.4.3
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Please see the attached patch, does it look better to you?

[Pavel Březina]

On 09/01/2015 11:19 AM, Jakub Hrozek wrote:
> On Wed, Aug 26, 2015 at 02:21:23PM +0200, Pavel Březina wrote:
> > On 08/18/2015 03:35 PM, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > one more usage of sss_unique_file I forgot to send earlier.
> >
> > Nack.
> > >   ret = close(fd);
> > > fd = -1;
> > >  if (ret == -1) {
> > >      ret = errno;
> > >      DEBUG(SSSDBG_CRIT_FAILURE,
> > >
> > > @@ -369,12 +366,16 @@ static errno_t gpo_cache_store_file(const char *smb_path,
> > >           goto done;
> > >       }
> >
> > Please, move this close(fd) call to done part, so we close fd only there.
> > > ret = EOK;
> > > done:
> > > if (ret != EOK) {
> > >   DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
> > > }
> >
> > And when you are here, can you fix indentation in the debug statement above?
> > > if (fd != -1) {
> > >    close(fd);
> > > }
> > > talloc_free(tmp_ctx);
> > > return ret;
> > > }
> > >
> > > -- 2.4.3
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Please see the attached patch, does it look better to you?

Hi, yes. There is just an indentation error:
> if (fd != -1) {
>    fret = close(fd);
>    if (fret == -1) {
>        fret = errno;
>        DEBUG(SSSDBG_CRIT_FAILURE,
>            "close failed [%d][%s].\n", fret, strerror(fret));

^^
>    }
> }

[Jakub Hrozek]

On Tue, Sep 01, 2015 at 11:28:32AM +0200, Pavel Březina wrote:
> > Please see the attached patch, does it look better to you?
>
> Hi, yes. There is just an indentation error:
> > if (fd != -1) {
> >    fret = close(fd);
> >    if (fret == -1) {
> >        fret = errno;
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >            "close failed [%d][%s].\n", fret, strerror(fret));
>                ^^
>
> >    }

Let's try it again :-)

[Pavel Březina]

On 09/01/2015 11:54 AM, Jakub Hrozek wrote:
> On Tue, Sep 01, 2015 at 11:28:32AM +0200, Pavel Březina wrote:
> > > Please see the attached patch, does it look better to you?
> >
> > Hi, yes. There is just an indentation error:
> > > if (fd != -1) {
> > >    fret = close(fd);
> > >    if (fret == -1) {
> > >        fret = errno;
> > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > >            "close failed [%d][%s].\n", fret, strerror(fret));
> >                 ^^
> >
> > >    }
>
> Let's try it again :-)

Ack.

[Jakub Hrozek]

On Tue, Sep 01, 2015 at 12:57:14PM +0200, Pavel Březina wrote:
> On 09/01/2015 11:54 AM, Jakub Hrozek wrote:
> > On Tue, Sep 01, 2015 at 11:28:32AM +0200, Pavel Březina wrote:
> > > > Please see the attached patch, does it look better to you?
> > >
> > > Hi, yes. There is just an indentation error:
> > > > if (fd != -1) {
> > > >    fret = close(fd);
> > > >    if (fret == -1) {
> > > >        fret = errno;
> > > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > > >            "close failed [%d][%s].\n", fret, strerror(fret));
> > >                ^^
> > >
> > > >    }
> >
> > Let's try it again :-)
>
> Ack.

Thanks for the review, pushed to master:
3954cd07dae78bf79136f0854472757d1ed26897