Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better error message for unfound new slave domains #3051

Merged

Conversation

pieterlexis
Copy link
Contributor

Closes #2405

@@ -818,7 +820,10 @@ void Bind2Backend::loadConfig(string* status)
}
catch(std::exception &ae) {
ostringstream msg;
msg<<" error at "+nowTime()+" parsing '"<<i->name.toString()<<"' from file '"<<i->filename<<"': "<<ae.what();
if (strcmp(ae.what(), "No such file or directory") && isNew && i->type == "slave")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That strcmp smells bad :)

@cmouse
Copy link
Contributor

cmouse commented Dec 21, 2015

My eyes :(

@Habbie Habbie added this to the auth-4-alpha1 milestone Dec 22, 2015
@pieterlexis pieterlexis force-pushed the issue-2405-misleading-error-in-bind branch 2 times, most recently from 8fe8cff to b2ceb76 Compare January 5, 2016 12:47
@pieterlexis
Copy link
Contributor Author

Could use another review

@cmouse
Copy link
Contributor

cmouse commented Jan 12, 2016

LGTM. 👍

@zeha
Copy link
Collaborator

zeha commented Jan 12, 2016

Doing the errno check that late is probably unsafe/unportable.

@cmouse
Copy link
Contributor

cmouse commented Jan 12, 2016

Could you catch http://en.cppreference.com/w/cpp/io/ios_base/failure instead? It comes with error code. Or std::system_error, has code as well.

@pieterlexis pieterlexis force-pushed the issue-2405-misleading-error-in-bind branch from b2ceb76 to cfbc538 Compare January 20, 2016 10:58
@pieterlexis
Copy link
Contributor Author

The zoneparser now throws a std::system_error and as that is a subclass of std::exception all other catches still work.

@Habbie
Copy link
Member

Habbie commented Feb 4, 2016

zoneparser-tng.cc: In member function ‘void ZoneParserTNG::stackFile(const string&)’:
zoneparser-tng.cc:59:5: error: ‘error_code’ is not a member of ‘std’
     std::error_code ec (errno,std::generic_category());
     ^
zoneparser-tng.cc:59:21: error: expected ‘;’ before ‘ec’
     std::error_code ec (errno,std::generic_category());
                     ^
zoneparser-tng.cc:60:11: error: ‘system_error’ is not a member of ‘std’
     throw std::system_error(ec, "Unable to open file '"+fname+"': "+stringerror());
           ^
zoneparser-tng.cc:60:29: error: ‘ec’ was not declared in this scope
     throw std::system_error(ec, "Unable to open file '"+fname+"': "+stringerror());
                             ^
make[3]: *** [zoneparser-tng.o] Error 1

@pieterlexis pieterlexis force-pushed the issue-2405-misleading-error-in-bind branch from cfbc538 to 1c18a27 Compare February 9, 2016 09:58
@pieterlexis
Copy link
Contributor Author

std::errorcode is c++2011, interesting that it fails on travis

@Habbie
Copy link
Member

Habbie commented Feb 16, 2016

LGTM (pending figuring out why Travis is throwing a fit)

@nlyan
Copy link
Contributor

nlyan commented Feb 16, 2016

Can't help with the build issue on travis, but I'd recommend deploying a function like this somewhere to avoid code bloat with system_error

namespace  { // or mark it static
template <typename Message> __attribute__ ((noinline, cold, noreturn)) 
void throw_errno (Message&& msg) {
    throw std::system_error (errno, std::system_category(), 
              std::forward<Message> (msg));
}
}

You can see what kind of codegen you get without it, even at -O2, here: http://goo.gl/Kzfcsw

@mind04
Copy link
Contributor

mind04 commented Jul 4, 2016

Please note the small warning sign on the cplusplus.com page:

This page describes a feature introduced by the latest revision of the C++ standard (2011). Older compilers may not support it.

@rgacogne
Copy link
Member

rgacogne commented Jul 4, 2016

It seems to work by adding #include <system_error> to both modified files.

ostringstream msg;
msg<<" error at "+nowTime()+" parsing '"<<i->name.toString()<<"' from file '"<<i->filename<<"': "<<ae.what();
if (ae.code().value() == ENOENT && isNew && i->type == "slave")
msg<<" error at "+nowTime()<<" no file found for new slave domain '"<<i->name.toString()<<"'. Has not been AXFR'd yet";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't the logger support printing DNSName w/o toString today?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when #4062 is merged, ostreams support this properly

@pieterlexis pieterlexis force-pushed the issue-2405-misleading-error-in-bind branch from 1c18a27 to 05f3f62 Compare July 4, 2016 13:38
@pieterlexis pieterlexis force-pushed the issue-2405-misleading-error-in-bind branch from 05f3f62 to e0e48a7 Compare July 4, 2016 15:03
@pieterlexis pieterlexis merged commit 523d7bf into PowerDNS:master Jul 6, 2016
@pieterlexis pieterlexis deleted the issue-2405-misleading-error-in-bind branch July 6, 2016 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants