Patch sent for review.
On 09/07/2012 05:53 AM, Ariel Barria wrote:
Patch sent for review.
https://fedorahosted.org/sssd/ticket/1371
sssd-devel mailing list sssd-devel@lists.fedorahosted.org 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
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 sssd-devel@lists.fedorahosted.org 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 */ }
thanks to both for comments.
Date: Fri, 7 Sep 2012 14:02:39 +0200 From: jhrozek@redhat.com To: sssd-devel@lists.fedorahosted.org 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 sssd-devel@lists.fedorahosted.org 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 sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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))); + }
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 sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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
Date: Thu, 13 Sep 2012 19:21:17 +0200 From: mzidek@redhat.com To: sssd-devel@lists.fedorahosted.org 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 sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
new patch. thanks again.
Date: Thu, 13 Sep 2012 19:21:17 +0200 From: mzidek@redhat.com To: sssd-devel@lists.fedorahosted.org 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 sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 09/14/2012 05:40 AM, Ariel Barria wrote:
new patch. thanks again.
Date: Thu, 13 Sep 2012 19:21:17 +0200 From: mzidek@redhat.com To: sssd-devel@lists.fedorahosted.org 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 sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org 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
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: mzidek@redhat.com To: sssd-devel@lists.fedorahosted.org 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 sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org 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)
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 sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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!
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.
sssd-devel@lists.fedorahosted.org