Re: [SyncEvolution] Memotoo tasks: PERCENT-COMPLETE:100
Le 19/12/2011 09:20, Patrick Ohly a écrit : Hello Thomas! Helllo Patrick ! I ran a new full test over the weekend and found one issue: --- BEGIN:VCALENDAR BEGIN:VCALENDAR VERSION:2.0 VERSION:2.0 BEGIN:VTODO BEGIN:VTODO SUMMARY:test task with plenty of fi SUMMARY:test task with plenty of fi elds elds CATEGORIES:Business,Waiting CATEGORIES:Business,Waiting COMPLETED:20090401T05COMPLETED:20090401T05 DESCRIPTION:This is a test task in DESCRIPTION:This is a test task in time zone New York\, to be started time zone New York\, to be started May 1st 2009 (not supported by Memo May 1st 2009 (not supported by Memo too)\, due Mai 2st 2009.\n\nIt uses too)\, due Mai 2st 2009.\n\nIt uses non-standard status values. non-standard status values. DUE:20090502T19 DUE:20090502T19 >PERCENT-COMPLETE:100 STATUS:COMPLETED STATUS:COMPLETED END:VTODOEND:VTODO END:VCALENDAREND:VCALENDAR --- It seems that Memotoo adds a PERCENT-COMPLETE:100. The original data has no PERCENT-COMPLETE when testing with Memotoo (see below). The standard doesn't say anything about a default value for PERCENT-COMPLETE; my own intuition suggests that "nothing said = nothing done = PERCENT-COMPLETE:0" would be a better choice than 100% done. Has Memotoo started to support PERCENT-COMPLETE? What about DTSTART? Memotoo add "PERCENT-COMPLETE:100" if the date "COMPLETED" is set. DTSTART is supported by Memotoo The Memotoo version of the task test case was modified a while ago to not have PERCENT-COMPLETE and DTSTART: "PERCENT-COMPLETE" is set in Memotoo since many months ... commit 358e8fc03b60ad6e34fb1815bf20694745ce8571 Author: Patrick Ohly Date: Wed Aug 10 16:02:30 2011 +0200 Memotoo testing: updated eds_task test case for Memotoo Time stamp for COMPLETED is converted to local time by Memotoo. DTSTART and PERCENT-COMPLETE are lost. ___ SyncEvolution mailing list SyncEvolution@syncevolution.org http://lists.syncevolution.org/listinfo/syncevolution
Re: [SyncEvolution] syncevo-dbus-server + forking processes
On Mon, Dec 19, 2011 at 6:02 PM, Patrick Ohly wrote:
> On Mo, 2011-12-19 at 16:47 +0100, Chris Kühl wrote:
>> On Mon, Dec 19, 2011 at 12:08 PM, Patrick Ohly
>> wrote:
>> > So perhaps the parameter for that can be removed and made a static
>> > choice in the dbus_get_bus_connection() implementations?
>> >
>>
>> I'm about half way through test-dbus.py and things are looking fine
>> using the singleton connection acquired by g_bus_get_sync(). so, if my
>> interpretation of what you're saying is correct, we can just remove
>> the unshared (btw, using a negative for a boolean value is confusing.)
>> option and make the gio gdbus always be a shared, singleton connection
>> and the libdbus connection always be private. Correct?
>
> Right.
>
>> > So let me be more precise: a "syncevolution --version" invocation
>> > segfaults.
>> >
>>
>> Hmm. I get SyncEvolution 1.2.1+20111218+SE+3f4f71a (pre-release) when
>> running that on my Debian Testing install. But we have had different
>> results in the past on Deban Testing so I'm becoming less and less
>> surprised by this.
>
> Well, Testing is a moving target ;-) We might also be compiling
> differently, or there is a 32/64 bit difference (I'm using 64 bit).
>
> Anyway, as I am to reproduce it, finding the root cause wasn't that
> difficult:
>
> commit b7b17159d9e89c0452e3df99a35668a468e347d7
> Author: Patrick Ohly
> Date: Mon Dec 19 17:57:20 2011 +0100
>
> GIO GDBus: fixed memory corruption
>
> g_variant_get() does not work for C++ bool directly because at least
> on x86_64, C++ bool uses 1 byte while gboolean uses 4. This caused
> random stack corruption when unpacking four bytes into an address
> which only had room for one. Caused "syncevolution --version" to segfault
> when using a corrupted std::string later.
>
> diff --git a/src/gdbusxx/gdbus-cxx-bridge.h b/src/gdbusxx/gdbus-cxx-bridge.h
> index 106b216..568642b 100644
> --- a/src/gdbusxx/gdbus-cxx-bridge.h
> +++ b/src/gdbusxx/gdbus-cxx-bridge.h
> @@ -1170,12 +1170,37 @@ template<> struct dbus_traits :
> static std::string getReply() { return ""; }
> };
>
> -template<> struct dbus_traits :
> - public basic_marshal< bool, VariantTypeBoolean >
> +template<> struct dbus_traits
> +// cannot use basic_marshal because VariantTypeBoolean packs/unpacks
> +// a gboolean, which is not a C++ bool (4 bytes vs 1 on x86_64)
> +// public basic_marshal< bool, VariantTypeBoolean >
> {
> static std::string getType() { return "b"; }
> static std::string getSignature() {return getType(); }
> static std::string getReply() { return ""; }
> +
> + typedef bool host_type;
> + typedef bool arg_type;
> +
> + static void get(GDBusConnection *conn, GDBusMessage *msg,
> + GVariantIter &iter, bool &value)
> + {
> + GVariant *var = g_variant_iter_next_value(&iter);
> + if (var == NULL || !g_variant_type_equal(g_variant_get_type(var),
> VariantTypeBoolean::getVariantType())) {
> + throw std::runtime_error("invalid argument");
> + }
> + gboolean buffer;
> + g_variant_get(var, g_variant_get_type_string(var), &buffer);
> + value = buffer;
> + g_variant_unref(var);
> + }
> +
> + static void append(GVariantBuilder &builder, bool value)
> + {
> + const gchar *typeStr =
> g_variant_type_dup_string(VariantTypeBoolean::getVariantType());
> + g_variant_builder_add(&builder, typeStr, (gboolean)value);
> + g_free((gpointer)typeStr);
> + }
> };
>
Oops, I recall removing that code.
Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution
Re: [SyncEvolution] syncevo-dbus-server + forking processes
On Mo, 2011-12-19 at 10:57 +0100, Chris Kühl wrote:
> On Fri, Dec 16, 2011 at 3:41 PM, Patrick Ohly wrote:
> > I have that same example working based on GDBus + libdbus. What I don't
> > like about it is that the connection is based on listen() + connect().
> > This makes it harder for the parent to detect that the child died: if
> > the parent did the usual trick (create socket pair, fork+exec child,
> > close its own side of the socket pair), then a closed socket would
> > indicate that the child died. With listen() + connect() it has to check
> > for existence of the child periodically. SIGCHILD does not integrate
> > into the main loop well, but I see that g_spawn() has a GChildWatch -
> > that should do.
> >
>
> Yes, looking at the source for the GChildWatch related code it blocks
> SIGCHLD. When the mainloop's worker routine is run it checks if any of
> the blocked signals are pending and then iterates over the
> GChildWatchSources checking if the process has exited. So by checking
> the return value of g_spawn_async_with_pipes and setting up the
> GChildWatch I don't see a way to miss the childs exit, either.
Attached is a first API proposal for the fork/exec helper class and how
it could be used in a small test program. Does that look sane?
I haven't started implementing any of the new methods, but it should be
fairly straightforward (famous last words).
> > While working on this I got unhappy with some aspects of the current
> > D-Bus C++ wrapper. The result is the "dbus-cleanup" branch. Chris, what
> > do you think about that?
>
> This looks fine. Moving the connection info to DBusObject definitely
> makes sense and reduces the code a bit.
Okay, merged into master.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
/*
* Copyright (C) 2011 Intel Corporation
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 2.1 of the License, or (at your option) version 3.
*
* This library is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this library; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301 USA
*/
#ifndef INCL_FORK_EXEC
# define INCL_FORK_EXEC
#ifdef HAVE_CONFIG_H
# include "config.h"
#endif
#ifdef HAVE_GLIB
#include
#include
#include
#include
SE_BEGIN_CXX
/**
* Utility class which starts a specific helper binary in a second
* process. The helper binary is identified via its base name like
* "syncevo-dbus-helper", exact location is then determined
* automatically, or via an absolute path.
*
* Direct D-Bus communication is set up automatically. For this to
* work, the helper must use ForkExecChild::connect().
*
* Progress (like "client connected") and failures ("client disconnected")
* are reported via boost::signal2 signals. To make progess, the user of
* this class must run a glib event loop.
*
* Note that failures encountered inside the class methods themselves
* will be reported via exceptions. Only asynchronous errors encountered
* inside the event loop are reported via the failure signal.
*/
class ForkExec : private boost::noncopyable {
public:
/**
* the glib main loop passed to create() or connect() in one of
* the derived classes
*/
GMainLoopPtr getLoop() const;
/**
* Called when the D-Bus connection is up and running. It is ready
* to register objects that the peer might need. It is
* guaranteed that any objects registered now will be ready before
* the helper gets a chance to make D-Bus calls.
*/
typedef boost::signals2::signal OnConnect;
OnConnect m_onConnect;
protected:
ForkExec(const GMainLoopPtr &loop);
};
/**
* The parent side of a fork/exec.
*/
class ForkExecParent : public ForkExec
{
public:
/**
* A ForkExecParent instance must be created via this factory
* method and then be tracked in a shared pointer. This method
* will not start the helper yet: first connect your slots, then
* call start().
*/
static boost::shared_ptr create(const GMainLoopPtr &loop,
const std::string &helper);
/**
* the helper string passed to create()
*/
std::string getHelper() const;
/**
* run the helper executable in the parent process
*/
void start();
/**
* request
Re: [SyncEvolution] syncevo-dbus-server + forking processes
On Mon, Dec 19, 2011 at 6:02 PM, Patrick Ohly wrote:
> On Mo, 2011-12-19 at 16:47 +0100, Chris Kühl wrote:
>> On Mon, Dec 19, 2011 at 12:08 PM, Patrick Ohly
>> wrote:
>> > So perhaps the parameter for that can be removed and made a static
>> > choice in the dbus_get_bus_connection() implementations?
>> >
>>
>> I'm about half way through test-dbus.py and things are looking fine
>> using the singleton connection acquired by g_bus_get_sync(). so, if my
>> interpretation of what you're saying is correct, we can just remove
>> the unshared (btw, using a negative for a boolean value is confusing.)
>> option and make the gio gdbus always be a shared, singleton connection
>> and the libdbus connection always be private. Correct?
>
> Right.
>
>> > So let me be more precise: a "syncevolution --version" invocation
>> > segfaults.
>> >
>>
>> Hmm. I get SyncEvolution 1.2.1+20111218+SE+3f4f71a (pre-release) when
>> running that on my Debian Testing install. But we have had different
>> results in the past on Deban Testing so I'm becoming less and less
>> surprised by this.
>
> Well, Testing is a moving target ;-) We might also be compiling
> differently, or there is a 32/64 bit difference (I'm using 64 bit).
>
> Anyway, as I am to reproduce it, finding the root cause wasn't that
> difficult:
>
> commit b7b17159d9e89c0452e3df99a35668a468e347d7
> Author: Patrick Ohly
> Date: Mon Dec 19 17:57:20 2011 +0100
>
> GIO GDBus: fixed memory corruption
>
> g_variant_get() does not work for C++ bool directly because at least
> on x86_64, C++ bool uses 1 byte while gboolean uses 4. This caused
> random stack corruption when unpacking four bytes into an address
> which only had room for one. Caused "syncevolution --version" to segfault
> when using a corrupted std::string later.
>
> diff --git a/src/gdbusxx/gdbus-cxx-bridge.h b/src/gdbusxx/gdbus-cxx-bridge.h
> index 106b216..568642b 100644
> --- a/src/gdbusxx/gdbus-cxx-bridge.h
> +++ b/src/gdbusxx/gdbus-cxx-bridge.h
> @@ -1170,12 +1170,37 @@ template<> struct dbus_traits :
> static std::string getReply() { return ""; }
> };
>
> -template<> struct dbus_traits :
Should this not be the following?
template<> struct dbus_traits : public dbus_traits_base
Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution
Re: [SyncEvolution] syncevo-dbus-server + forking processes
On Di, 2011-12-20 at 15:16 +0100, Chris Kühl wrote: > Should this not be the following? > > template<> struct dbus_traits : public dbus_traits_base Hmm, it compiled okay as I had it in my patch. Probably no code was ever expanded which checked for dbus_traits_base::asynchronous. Will fix that. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ___ SyncEvolution mailing list SyncEvolution@syncevolution.org http://lists.syncevolution.org/listinfo/syncevolution
