7:19 p.m.
http://bugzilla.moblin.org/show_bug.cgi?id=6548
Summary: D-Bus API: add specific exceptions
Classification: Moblin Projects
Product: SyncEvolution
Version: upstream
Platform: Netbook
OS/Version: Moblin Linux
Status: ASSIGNED
Severity: normal
Priority: P1
Component: * Feature Request
AssignedTo: patrick.ohly(a)intel.com
ReportedBy: patrick.ohly(a)intel.com
CC: syncevolution(a)lists.intel.com
Release Milestone: Undecided
<doc:para>
Method calls may fail with the following errors:
"org.syncevolution.Exception" - catch-all error condition.
"org.syncevolution.NoSuchConfig" - server configuration name is invalid
"org.syncevolution.NoSuchSource" - source name is invalid
"org.syncevolution.InvalidCall" - a call is (perhaps no longer) allowed
or suitable in the current situation, like Detach() when the client
is not attached.
</doc:para>
org.syncevolution.Exception is already used in the new
syncevo-dbus-server, in all cases. So we need to turn some of the other
cases into more specific errors.
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
11:23 p.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
pohly <patrick.ohly(a)intel.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Blocks| |7210
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
2:20 p.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
yongsheng zhu <yongsheng.zhu(a)intel.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |yongsheng.zhu(a)intel.com
--- Comment #1 from yongsheng zhu <yongsheng.zhu(a)intel.com> 2009-11-01 19:20:50
---
"org.syncevolution.NoSuchConfig" - server
configuration name is invalid
"org.syncevolution.NoSuchSource" - source name is invalid
"org.syncevolution.InvalidCall" - a call is (perhaps no longer)
allowed
or suitable in the current situation, like Detach() when the client
is not attached.
For these specific errors, I'll defined child classes
based on
'std::runtime_error' to distinguish them.
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
9:29 p.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
--- Comment #2 from pohly <patrick.ohly(a)intel.com> 2009-11-02 02:29:02 ---
(In reply to comment #1)
> "org.syncevolution.NoSuchConfig" - server
configuration name is invalid
> "org.syncevolution.NoSuchSource" - source name is invalid
> "org.syncevolution.InvalidCall" - a call is (perhaps no longer)
allowed
> or suitable in the current situation, like Detach() when the client
> is not attached.
For these specific errors, I'll defined child classes based on
'std::runtime_error' to distinguish them.
Better subclass SyncEvo::Exception from util.h. This includes source file and
line information, which is written into the log file.
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
5:07 p.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
--- Comment #3 from yongsheng zhu <yongsheng.zhu(a)intel.com> 2009-11-08 22:07:30
---
(In reply to comment #2)
(In reply to comment #1)
> > "org.syncevolution.NoSuchConfig" - server configuration name
is invalid
> > "org.syncevolution.NoSuchSource" - source name is invalid
> > "org.syncevolution.InvalidCall" - a call is (perhaps no
longer) allowed
> > or suitable in the current situation, like Detach() when the client
> > is not attached.
> For these specific errors, I'll defined child classes based on
> 'std::runtime_error' to distinguish them.
Better subclass SyncEvo::Exception from util.h. This includes source file and
line information, which is written into the log file.
I use subclass
SyncEvo::Exception. The patch is ready on 'origin/yongsheng'.
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
7:04 p.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
--- Comment #4 from pohly <patrick.ohly(a)intel.com> 2009-11-09 00:03:59 PST ---
(In reply to comment #3)
The patch is ready on 'origin/yongsheng'.
Did you see this in my recent email?
This change points towards a conceptual problem:
-example_LDADD = libgdbus.la @GLIB_LIBS@ @DBUS_LIBS@
+example_LDADD = libgdbus.la @GLIB_LIBS@ @DBUS_LIBS@
../syncevo/libsyncevolution.la
Nothing in the C++ binding should depend on SyncEvolution. Throwing a
"org.syncevolution.Exception" already was conceptually wrong, but at
least it always compiled cleanly, regardless whether libsyncevolution.la
was built first.
Can you come up with a solution where the exception handler inside gdbus
only returns a generic error (perhaps
org.syncevolution.gdbus-cxx.Exception, because org.syncevolution is
where the C++ binding is developed) and code inside SyncEvolution
extends the exception handling?
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
1:01 a.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
--- Comment #5 from yongsheng zhu <yongsheng.zhu(a)intel.com> 2009-11-09 06:01:05 PST
---
(In reply to comment #4)
(In reply to comment #3)
> The patch is ready on 'origin/yongsheng'.
Did you see this in my recent email?
This change points towards a conceptual problem:
-example_LDADD = libgdbus.la @GLIB_LIBS@ @DBUS_LIBS@
+example_LDADD = libgdbus.la @GLIB_LIBS@ @DBUS_LIBS@
../syncevo/libsyncevolution.la
Nothing in the C++ binding should depend on SyncEvolution. Throwing a
"org.syncevolution.Exception" already was conceptually wrong, but at
least it always compiled cleanly, regardless whether libsyncevolution.la
was built first.
Yes, I also think it is not a good way to involve dependency on
syncevolution
for gdbus c++ binding. I'll try to figure out a new solution.
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
4:12 p.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
--- Comment #6 from yongsheng zhu <yongsheng.zhu(a)intel.com> 2009-11-09 21:12:00 PST
---
see new commit. Patrick, please review it.
commit ac94948efc18a5b6c3281c00fa077edbf8b3a953
Author: Zhu, Yongsheng <yongsheng.zhu(a)intel.com>
Date: Mon Nov 2 13:42:07 2009 +0800
syncevo-dbus-server: add specific exceptions (MB#6548)
Define a DBusCxxException class in gdbus cxx bridge.
It only has 3 virtual functions: getName, getMessage,
handleException. These 3 functions are used in exception
handling and help avoid gdbus depending on syncevolution.
Then define a class DBusSyncException deriving from
DBusCxxException and SyncEvo::Exception.
Also 3 classes NoSuchConfig, NoSuchSource,
InvalidCall are based on DBusSyncException.
Turn some errors into these 3 specific errors.
There are still many exceptions needed to throw
new exceptions instead of 'runtime_error'.
Should be fixed in future.
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
7:59 p.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
--- Comment #7 from pohly <patrick.ohly(a)intel.com> 2009-11-10 00:59:19 PST ---
(In reply to comment #6)
There are still many exceptions needed to throw
new exceptions instead of 'runtime_error'.
Should be fixed in future.
Are you suggesting that all exceptions ever thrown in SyncEvolution should be
derived from DBusSyncException? That would again introduce an unwanted
dependency, now in the core code.
I'm asking because right now, SyncEvo::Exception is treated like a
std::runtime_exception in the D-Bus bridge. It is not logged and reported as
"org.syncevolution.gdbus-cxx.Exception".
How about this approach:
In gdbus-cxx-bridge.h:
static DBusMessage *handleException(DBusMessage *msg)
{
try {
#ifdef DBUS_CXX_EXCEPTION_HANDLER
return DBUS_CXX_EXCEPTION_HANDLER(msg);
#else
throw;
#endif
} catch (const std::runtime_error &ex) {
return g_dbus_create_error(msg,
"org.syncevolution.gdbus-cxx.Exception", "%s", ex.what());
} catch (...) {
return g_dbus_create_error(msg,
"org.syncevolution.gdbus-cxx.Exception", "unknown");
}
}
In syncevo-dbus-server.cpp:
static DBusMessage *SyncEvoHandleException(DBusMessage *msg);
#define DBUS_CXX_EXCEPTION_HANDLER SyncEvoHandleException
#include <gdbus-cxx-bridge.h>
...
static DBusMessage *SyncEvoHandleException(DBusMessage *msg);
{
Exception::handle(); // for logging
try {
throw;
} catch const DBusCXXException &ex) {
return g_dbus_create_error(msg, ex.getName().c_str(), "%s",
ex.getMessage());
} catch } catch (const std::runtime_error &ex) {
return g_dbus_create_error(msg, "org.syncevolution.Exception",
"%s",
ex.what());
} catch (...) {
return g_dbus_create_error(msg, "org.syncevolution.Exception",
"unknown");
}
}
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
8:33 p.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
--- Comment #8 from yongsheng zhu <yongsheng.zhu(a)intel.com> 2009-11-10 01:33:12 PST
---
(In reply to comment #7)
(In reply to comment #6)
> There are still many exceptions needed to throw
> new exceptions instead of 'runtime_error'.
> Should be fixed in future.
Are you suggesting that all exceptions ever thrown in SyncEvolution should be
derived from DBusSyncException? That would again introduce an unwanted
dependency, now in the core code.
Yes, you are right.
How about this approach:
In gdbus-cxx-bridge.h:
static DBusMessage *handleException(DBusMessage *msg)
{
try {
#ifdef DBUS_CXX_EXCEPTION_HANDLER
return DBUS_CXX_EXCEPTION_HANDLER(msg);
#else
throw;
#endif
} catch (const std::runtime_error &ex) {
return g_dbus_create_error(msg,
"org.syncevolution.gdbus-cxx.Exception", "%s", ex.what());
} catch (...) {
return g_dbus_create_error(msg,
"org.syncevolution.gdbus-cxx.Exception", "unknown");
}
}
In syncevo-dbus-server.cpp:
static DBusMessage *SyncEvoHandleException(DBusMessage *msg);
#define DBUS_CXX_EXCEPTION_HANDLER SyncEvoHandleException
#include <gdbus-cxx-bridge.h>
...
static DBusMessage *SyncEvoHandleException(DBusMessage *msg);
{
Exception::handle(); // for logging
try {
throw;
} catch const DBusCXXException &ex) {
return g_dbus_create_error(msg, ex.getName().c_str(), "%s",
ex.getMessage());
} catch } catch (const std::runtime_error &ex) {
return g_dbus_create_error(msg, "org.syncevolution.Exception",
"%s",
ex.what());
} catch (...) {
return g_dbus_create_error(msg, "org.syncevolution.Exception",
"unknown");
}
}
workable. Glib also uses this method heavily.
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
11:15 p.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
--- Comment #9 from yongsheng zhu <yongsheng.zhu(a)intel.com> 2009-11-10 04:15:14 PST
---
Done. Remove dependency on changing all runtime_errors to DBusExceptions
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
12:07 a.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
pohly <patrick.ohly(a)intel.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |RESOLVED
Resolution| |FIXED
--- Comment #10 from pohly <patrick.ohly(a)intel.com> 2009-11-10 05:07:37 PST ---
(In reply to comment #9)
Done. Remove dependency on changing all runtime_errors to
DBusExceptions
Merged, with slightly updated commit message (for example, still listed the
removed handleException method).
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
12:33 a.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
pohly <patrick.ohly(a)intel.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |VERIFIED
--- Comment #11 from pohly <patrick.ohly(a)intel.com> 2009-11-10 05:33:29 PST ---
At this point, I consider new features incomplete without a corresponding unit
test. To verify this issue I wrote one.
When looking at the source, I found that the read-only Server.GetConfig() has
the side effect of creating a configuration from a template when a server
configuration of the given name does not exist. As discussed before, this is
not part of the D-Bus API spec and needs to be removed.
I thought we had removed that before merging the implementation, but this one
must have slipped through. I removed it on master.
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
12:01 p.m.
New subject: [Bug 6548] D-Bus API: add specific exceptions
http://bugzilla.moblin.org/show_bug.cgi?id=6548
--- Comment #12 from yongsheng zhu <yongsheng.zhu(a)intel.com> 2009-11-10 17:01:05
PST ---
(In reply to comment #11)
At this point, I consider new features incomplete without a
corresponding unit
test. To verify this issue I wrote one.
When looking at the source, I found that the read-only Server.GetConfig() has
the side effect of creating a configuration from a template when a server
configuration of the given name does not exist. As discussed before, this is
not part of the D-Bus API spec and needs to be removed.
I thought we had removed that before merging the implementation, but this one
must have slipped through. I removed it on master.
Yes, tests are important now.
I'm trying to write more cases this week.
--
Configure bugmail: http://bugzilla.moblin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
4694
days inactive
4730
days old
syncevolution-issues@syncevolution.org
13 comments
1 participants
participants (1)
-
bugzilla＠moblin.org