[PATCH] 1371-Missing resolv.conf should be non-fatal

[Ariel Barria]

Patch sent for review.

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

[Michal Židek]

On 09/07/2012 05:53 AM, Ariel Barria wrote:
> Patch sent for review.
>
> https://fedorahosted.org/sssd/ticket/1371
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Hi Ariel,

I have not tested your patch, but I see some problems in it.
Do not set errno variable explicitly to 0. It is not how it is ment
to be used. Also ENOENT is not the only possible error, we
should check for the other errors as well. Also do not clear
the return value of stat.

The check should look something like:

ret = stat( ... );
if (ret != 0) {
     if (errno == ENOENT) {
         /* resolv.conf not found. This is the reason for this patch ...*/;
     } else {
         /* ... but we must check for other errors as well
          (the error msg here can be more general) */
     }
} else {
     /* Here is everything OK */
}

Thanks
Michal

[Jakub Hrozek]

On Fri, Sep 07, 2012 at 12:41:30PM +0200, Michal Židek wrote:
> On 09/07/2012 05:53 AM, Ariel Barria wrote:
> > Patch sent for review.
> >
> > https://fedorahosted.org/sssd/ticket/1371
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Hi Ariel,
>
> I have not tested your patch, but I see some problems in it.
> Do not set errno variable explicitly to 0. It is not how it is ment
> to be used.

Actually, it might be beneficial to clear errno in some cases. stat(2)
is probably not one of them as it only sets errno in an error condition.

Here is a nice summary on the errno checking:
https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=66...
> Also ENOENT is not the only possible error, we
> should check for the other errors as well. Also do not clear
> the return value of stat.
>
> The check should look something like:
>
> ret = stat( ... );
> if (ret != 0) {

I would also assign the errno value to ret immediatelly to avoid
rewriting errno with another DEBUG call for instance.
> if (errno == ENOENT) {
>     /* resolv.conf not found. This is the reason for this patch ...*/;
> } else {
>     /* ... but we must check for other errors as well
>      (the error msg here can be more general) */
> }
>
> } else {
>     /* Here is everything OK */
> }

[Ariel Barria]

thanks to both for comments.
> Date: Fri, 7 Sep 2012 14:02:39 +0200
> From: [e-mail]
> To: [e-mail]
> Subject: Re: [SSSD] [PATCH] 1371-Missing resolv.conf should be non-fatal
>
> On Fri, Sep 07, 2012 at 12:41:30PM +0200, Michal Židek wrote:
> > On 09/07/2012 05:53 AM, Ariel Barria wrote:
> > > Patch sent for review.
> > >
> > > https://fedorahosted.org/sssd/ticket/1371
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Hi Ariel,
> >
> > I have not tested your patch, but I see some problems in it.
> > Do not set errno variable explicitly to 0. It is not how it is ment
> > to be used.
>
> Actually, it might be beneficial to clear errno in some cases. stat(2)
> is probably not one of them as it only sets errno in an error condition.
>
> Here is a nice summary on the errno checking:
> https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=66...
> > Also ENOENT is not the only possible error, we
> > should check for the other errors as well. Also do not clear
> > the return value of stat.
> >
> > The check should look something like:
> >
> > ret = stat( ... );
> > if (ret != 0) {
>
> I would also assign the errno value to ret immediatelly to avoid
> rewriting errno with another DEBUG call for instance.
> > if (errno == ENOENT) {
> >     /* resolv.conf not found. This is the reason for this patch ...*/;
> > } else {
> >     /* ... but we must check for other errors as well
> >      (the error msg here can be more general) */
> > }
> >
> > } else {
> >     /* Here is everything OK */
> > }
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, Sep 10, 2012 at 09:21:05PM -0500, Ariel Barria wrote:
> thanks to both for comments.

Hi Ariel,

actually I reread the patch and I think there is a simpler way to fix
the ticket..directly in the monitor_config_file() function, there is
already a stat() call:
1823     ret = stat(file, &file_stat);
1824     if (ret < 0) {
1825         err = errno;
1826         DEBUG(0, ("Could not stat file [%s]. Error [%d:%s]\n",
1827                   file, err, strerror(err)));
1828         return err;
1829     }

So I think that simply extending the error handler with special-casing
ENOENT as you had in your patch would work fine:
+        if (ret == ENOENT) {
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                 ("file [%s] is missing, but will skip this check.\n",
+                 RESOLV_CONF_PATH));
+        } else {
+            DEBUG(SSSDBG_MINOR_FAILURE,("Could not stat file [%s]. Error [%d:%s]\n",
+                  RESOLV_CONF_PATH, ret, strerror(ret)));
+        }

[Ariel Barria]

hi, new patch :).

Apparently was required also monitor_config_file_fallback().
> Hi Ariel,
>
> actually I reread the patch and I think there is a simpler way to fix
> the ticket..directly in the monitor_config_file() function, there is
> already a stat() call:
> 1823     ret = stat(file, &file_stat);
> 1824     if (ret < 0) {
> 1825         err = errno;
> 1826         DEBUG(0, ("Could not stat file [%s]. Error [%d:%s]\n",
> 1827                   file, err, strerror(err)));
> 1828         return err;
> 1829     }
>
> So I think that simply extending the error handler with special-casing
> ENOENT as you had in your patch would work fine:
>    if (ret == ENOENT) {
>        DEBUG(SSSDBG_MINOR_FAILURE,
>             ("file [%s] is missing, but will skip this check.\n",
>             RESOLV_CONF_PATH));
>    } else {
>        DEBUG(SSSDBG_MINOR_FAILURE,("Could not stat file [%s]. Error [%d:%s]\n",
>              RESOLV_CONF_PATH, ret, strerror(ret)));
>    }
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Michal Židek]

On 09/13/2012 05:18 AM, Ariel Barria wrote:
> hi, new patch :).
>
> Apparently was required also monitor_config_file_fallback().
> > Hi Ariel,
> >
> > actually I reread the patch and I think there is a simpler way to fix
> > the ticket..directly in the monitor_config_file() function, there is
> > already a stat() call:
> > 1823 ret = stat(file, &file_stat);
> > 1824 if (ret < 0) {
> > 1825 err = errno;
> > 1826 DEBUG(0, ("Could not stat file [%s]. Error [%d:%s]\n",
> > 1827 file, err, strerror(err)));
> > 1828 return err;
> > 1829 }
> >
> > So I think that simply extending the error handler with special-casing
> > ENOENT as you had in your patch would work fine:
> > if (ret == ENOENT) {
> > DEBUG(SSSDBG_MINOR_FAILURE,
> > ("file [%s] is missing, but will skip this check.\n",
> > RESOLV_CONF_PATH));
> > } else {
> > DEBUG(SSSDBG_MINOR_FAILURE,("Could not stat file [%s]. Error
>
> [%d:%s]\n",
> > RESOLV_CONF_PATH, ret, strerror(ret)));
> > }

Hi Ariel,

you should always check the return value of the stat function
and not only if the 'verify_exists_file' is false (also please change
the name of this parameter to something like 'ignore_missing', I find
'verify_exists_file' a bit obfuscated).

If 'ignore_missing' is set to false and the file is missing, SSSD will
exit with error. If it is true, it will tolerate ENOENT and continue
(but still we need to check the return value and print appropriate
debug message). Also ENOENT is not the only possible error and whatever
ignore_missing is set to, we must not allow SSSD continue if they
occur).

In other words. If ignore_missing is set to false, we do not tolerate
any errors, if it is true, we tolerate ENOENT.

Also please add this new parameter to the ifdefed call to the
monitor_config_file (currently on line 2031 in monitor.c).

Thanks
Michal

[Ariel Barria]

> Date: Thu, 13 Sep 2012 19:21:17 +0200
> From: [e-mail]
> To: [e-mail]
> Subject: Re: [SSSD] [PATCH] 1371-Missing resolv.conf should be non-fatal
>
> On 09/13/2012 05:18 AM, Ariel Barria wrote:
> > hi, new patch :).
> >
> > Apparently was required also monitor_config_file_fallback().
> > > Hi Ariel,
> > >
> > > actually I reread the patch and I think there is a simpler way to fix
> > > the ticket..directly in the monitor_config_file() function, there is
> > > already a stat() call:
> > > 1823 ret = stat(file, &file_stat);
> > > 1824 if (ret < 0) {
> > > 1825 err = errno;
> > > 1826 DEBUG(0, ("Could not stat file [%s]. Error [%d:%s]\n",
> > > 1827 file, err, strerror(err)));
> > > 1828 return err;
> > > 1829 }
> > >
> > > So I think that simply extending the error handler with special-casing
> > > ENOENT as you had in your patch would work fine:
> > > if (ret == ENOENT) {
> > > DEBUG(SSSDBG_MINOR_FAILURE,
> > > ("file [%s] is missing, but will skip this check.\n",
> > > RESOLV_CONF_PATH));
> > > } else {
> > > DEBUG(SSSDBG_MINOR_FAILURE,("Could not stat file [%s]. Error
> >
> > [%d:%s]\n",
> > > RESOLV_CONF_PATH, ret, strerror(ret)));
> > > }
>
> Hi Ariel,
>
> you should always check the return value of the stat function
> and not only if the 'verify_exists_file' is false (also please change
> the name of this parameter to something like 'ignore_missing', I find
> 'verify_exists_file' a bit obfuscated).

ok thanks :) 
 I will change the name, but the review is always done in this pathc. it existed
     ret = stat(file, &file_stat);
     if (ret < 0) {
         err = errno;
         if (!verify_exists_file) {
            if (err == ENOENT) {

this so does. only do I not was change the description. i will change.
> If 'ignore_missing' is set to false and the file is missing, SSSD will
> exit with error. If it is true, it will tolerate ENOENT and continue
> (but still we need to check the return value and print appropriate
> debug message). Also ENOENT is not the only possible error and whatever
> ignore_missing is set to, we must not allow SSSD continue if they
> occur).
>
> In other words. If ignore_missing is set to false, we do not tolerate
> any errors, if it is true, we tolerate ENOENT.
>
> Also please add this new parameter to the ifdefed call to the
> monitor_config_file (currently on line 2031 in monitor.c).
>
> Thanks
> Michal
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Ariel Barria]

new patch.
thanks again.
> Date: Thu, 13 Sep 2012 19:21:17 +0200
> From: [e-mail]
> To: [e-mail]
> Subject: Re: [SSSD] [PATCH] 1371-Missing resolv.conf should be non-fatal
>
> On 09/13/2012 05:18 AM, Ariel Barria wrote:
> > hi, new patch :).
> >
> > Apparently was required also monitor_config_file_fallback().
> > > Hi Ariel,
> > >
> > > actually I reread the patch and I think there is a simpler way to fix
> > > the ticket..directly in the monitor_config_file() function, there is
> > > already a stat() call:
> > > 1823 ret = stat(file, &file_stat);
> > > 1824 if (ret < 0) {
> > > 1825 err = errno;
> > > 1826 DEBUG(0, ("Could not stat file [%s]. Error [%d:%s]\n",
> > > 1827 file, err, strerror(err)));
> > > 1828 return err;
> > > 1829 }
> > >
> > > So I think that simply extending the error handler with special-casing
> > > ENOENT as you had in your patch would work fine:
> > > if (ret == ENOENT) {
> > > DEBUG(SSSDBG_MINOR_FAILURE,
> > > ("file [%s] is missing, but will skip this check.\n",
> > > RESOLV_CONF_PATH));
> > > } else {
> > > DEBUG(SSSDBG_MINOR_FAILURE,("Could not stat file [%s]. Error
> >
> > [%d:%s]\n",
> > > RESOLV_CONF_PATH, ret, strerror(ret)));
> > > }
>
> Hi Ariel,
>
> you should always check the return value of the stat function
> and not only if the 'verify_exists_file' is false (also please change
> the name of this parameter to something like 'ignore_missing', I find
> 'verify_exists_file' a bit obfuscated).
>
> If 'ignore_missing' is set to false and the file is missing, SSSD will
> exit with error. If it is true, it will tolerate ENOENT and continue
> (but still we need to check the return value and print appropriate
> debug message). Also ENOENT is not the only possible error and whatever
> ignore_missing is set to, we must not allow SSSD continue if they
> occur).
>
> In other words. If ignore_missing is set to false, we do not tolerate
> any errors, if it is true, we tolerate ENOENT.
>
> Also please add this new parameter to the ifdefed call to the
> monitor_config_file (currently on line 2031 in monitor.c).
>
> Thanks
> Michal
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Michal Židek]

On 09/14/2012 05:40 AM, Ariel Barria wrote:
> new patch.
> thanks again.
> > Date: Thu, 13 Sep 2012 19:21:17 +0200
> > From: [e-mail]
> > To: [e-mail]
> > Subject: Re: [SSSD] [PATCH] 1371-Missing resolv.conf should be non-fatal
> >
> > On 09/13/2012 05:18 AM, Ariel Barria wrote:
> > > hi, new patch :).
> > >
> > > Apparently was required also monitor_config_file_fallback().
> > > > Hi Ariel,
> > > >
> > > > actually I reread the patch and I think there is a simpler way to fix
> > > > the ticket..directly in the monitor_config_file() function, there is
> > > > already a stat() call:
> > > > 1823 ret = stat(file, &file_stat);
> > > > 1824 if (ret < 0) {
> > > > 1825 err = errno;
> > > > 1826 DEBUG(0, ("Could not stat file [%s]. Error [%d:%s]\n",
> > > > 1827 file, err, strerror(err)));
> > > > 1828 return err;
> > > > 1829 }
> > > >
> > > > So I think that simply extending the error handler with
>
> special-casing
> > > > ENOENT as you had in your patch would work fine:
> > > > if (ret == ENOENT) {
> > > > DEBUG(SSSDBG_MINOR_FAILURE,
> > > > ("file [%s] is missing, but will skip this check.\n",
> > > > RESOLV_CONF_PATH));
> > > > } else {
> > > > DEBUG(SSSDBG_MINOR_FAILURE,("Could not stat file [%s]. Error
> > >
> > > [%d:%s]\n",
> > > > RESOLV_CONF_PATH, ret, strerror(ret)));
> > > > }
> >
> > Hi Ariel,
> >
> > you should always check the return value of the stat function
> > and not only if the 'verify_exists_file' is false (also please change
> > the name of this parameter to something like 'ignore_missing', I find
> > 'verify_exists_file' a bit obfuscated).
> >
> > If 'ignore_missing' is set to false and the file is missing, SSSD will
> > exit with error. If it is true, it will tolerate ENOENT and continue
> > (but still we need to check the return value and print appropriate
> > debug message). Also ENOENT is not the only possible error and whatever
> > ignore_missing is set to, we must not allow SSSD continue if they
> > occur).
> >
> > In other words. If ignore_missing is set to false, we do not tolerate
> > any errors, if it is true, we tolerate ENOENT.
> >
> > Also please add this new parameter to the ifdefed call to the
> > monitor_config_file (currently on line 2031 in monitor.c).
> >
> > Thanks
> > Michal
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

I read your code wrong last time, you checked the return value of stat
correctly. Sorry for the inconvenience.

ACK to the last incarnation of the patch.

Good work Ariel.

Michal

[Jakub Hrozek]

On Fri, Sep 14, 2012 at 11:26:15AM +0200, Michal Židek wrote:
> On 09/14/2012 05:40 AM, Ariel Barria wrote:
> > new patch.
> > thanks again.
> > > Date: Thu, 13 Sep 2012 19:21:17 +0200
> > > From: [e-mail]
> > > To: [e-mail]
> > > Subject: Re: [SSSD] [PATCH] 1371-Missing resolv.conf should be non-fatal
> > >
> > > On 09/13/2012 05:18 AM, Ariel Barria wrote:
> > > > hi, new patch :).
> > > >
> > > > Apparently was required also monitor_config_file_fallback().
> > > > > Hi Ariel,
> > > > >
> > > > > actually I reread the patch and I think there is a simpler way to fix
> > > > > the ticket..directly in the monitor_config_file() function, there is
> > > > > already a stat() call:
> > > > > 1823 ret = stat(file, &file_stat);
> > > > > 1824 if (ret < 0) {
> > > > > 1825 err = errno;
> > > > > 1826 DEBUG(0, ("Could not stat file [%s]. Error [%d:%s]\n",
> > > > > 1827 file, err, strerror(err)));
> > > > > 1828 return err;
> > > > > 1829 }
> > > > >
> > > > > So I think that simply extending the error handler with
> >
> > special-casing
> > > > > ENOENT as you had in your patch would work fine:
> > > > > if (ret == ENOENT) {
> > > > > DEBUG(SSSDBG_MINOR_FAILURE,
> > > > > ("file [%s] is missing, but will skip this check.\n",
> > > > > RESOLV_CONF_PATH));
> > > > > } else {
> > > > > DEBUG(SSSDBG_MINOR_FAILURE,("Could not stat file [%s]. Error
> > > >
> > > > [%d:%s]\n",
> > > > > RESOLV_CONF_PATH, ret, strerror(ret)));
> > > > > }
> > >
> > > Hi Ariel,
> > >
> > > you should always check the return value of the stat function
> > > and not only if the 'verify_exists_file' is false (also please change
> > > the name of this parameter to something like 'ignore_missing', I find
> > > 'verify_exists_file' a bit obfuscated).
> > >
> > > If 'ignore_missing' is set to false and the file is missing, SSSD will
> > > exit with error. If it is true, it will tolerate ENOENT and continue
> > > (but still we need to check the return value and print appropriate
> > > debug message). Also ENOENT is not the only possible error and whatever
> > > ignore_missing is set to, we must not allow SSSD continue if they
> > > occur).
> > >
> > > In other words. If ignore_missing is set to false, we do not tolerate
> > > any errors, if it is true, we tolerate ENOENT.
> > >
> > > Also please add this new parameter to the ifdefed call to the
> > > monitor_config_file (currently on line 2031 in monitor.c).
> > >
> > > Thanks
> > > Michal
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> I read your code wrong last time, you checked the return value of stat
> correctly. Sorry for the inconvenience.
>
> ACK to the last incarnation of the patch.
>
> Good work Ariel.
>
> Michal

Hi,

please consider squashing in the attached patch. I think it simplifies
the logic some more and just aborts the rest of the file watching
functions in case the config file is not there (which seems like a
logical behaviour)

[Ariel Barria]

it's true, simplifies quite a bit.   thanks :)> Hi,
> please consider squashing in the attached patch. I think it simplifies
> the logic some more and just aborts the rest of the file watching
> functions in case the config file is not there (which seems like a
> logical behaviour)

_______________________________________________
sssd-devel mailing list
[e-mail]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, Sep 17, 2012 at 09:39:28PM -0500, Ariel Barria wrote:
> it's true, simplifies quite a bit.   thanks :)> Hi,
> > please consider squashing in the attached patch. I think it simplifies
> > the logic some more and just aborts the rest of the file watching
> > functions in case the config file is not there (which seems like a
> > logical behaviour)

Ack, thank you!

[Jakub Hrozek]

On Wed, Sep 19, 2012 at 04:12:52PM +0200, Jakub Hrozek wrote:
> On Mon, Sep 17, 2012 at 09:39:28PM -0500, Ariel Barria wrote:
> > it's true, simplifies quite a bit.   thanks :)> Hi,
> > > please consider squashing in the attached patch. I think it simplifies
> > > the logic some more and just aborts the rest of the file watching
> > > functions in case the config file is not there (which seems like a
> > > logical behaviour)
>
> Ack, thank you!

Pushed to master.