Re: [os-libsynthesis] PHOTO + compare="never"
Hello Patrick, I'm picking the detail I can quickly answer right now :-) On 05.07.2013, at 11:58, Patrick Ohly wrote: > [...] > I broke MERGEFIELDS() when introducing that mode, see attached patch. It > is now still not backward-compatible. Is there a way to have a builtin > function with an optional parameter? There is, see the OPTVAL() in the param definitions, for example param_find in scriptcontext.cpp. In the function implementation, you can test for isAssigned() to see if the parameter was specified in the call or not. > If not, then should I introduce a > MERGEFIELDS() without parameter and a MERGEFIELDS_WITH_MODE(mode)? No, an optional mode parameter is better. Best Regards, Lukas ___ os-libsynthesis mailing list os-libsynthesis@synthesis.ch http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
Re: [os-libsynthesis] PHOTO + compare="never"
On Thu, 2013-07-04 at 15:56 +0200, Lukas Zeller wrote:
> On 04.07.2013, at 12:01, Patrick Ohly wrote:
>
> > I just noticed one aspect of the example configs that I wasn't aware of:
> > syncclient_sample_config.xml: > compare="never" merge="fillempty"/>
> >
> > [...]
> >
> > What was the rationale for using that mode for PHOTO? Is it for storages
> > which store photo data after re-encoding it?
>
> Exactly. For the mobile platforms I wrote clients, I never had an
> address book which did not somehow mangle (re-encode, resize, etc.)
> the data.
I see. For SyncEvolution, all local storages faithfully store the data
as-is, so it makes more sense to really compare data, even if it is more
costly.
Defining PHOTO as "blob" did not have any advantages for me, because in
SyncEvolution its content always comes from parsing a text item and thus
it is in memory anyway. I switched to "string" to get rid of the
"Winning and loosing Field 'PHOTO' not equal: '' <> ''" that I mentioned in my other email and to simplify the script
(now I can compare against EMPTY instead of having to check the size).
> > But with a storage that stores the data as-is, comparing it is
> > better (IMHO), because modified data actually gets stored. The downside
> > is that comparisons become more expensive.
>
> That's another reason. Comparison causes the BLOB contents to be
> pulled from storage (server side), which can be quite expensive.
Actually, it seems that comparisons are simply not done, instead giving
a sometimes false "does not match" result.
> > Speaking of comparisons, with EDS this is slightly tricky. Inlined data
> > gets replaced with a file reference, so what the comparison really needs
> > to do is not comparing
> > "file:///tmp/testing/temp-testpim/data/evolution/addressbook/pim-manager-testsync-testcontacts-foo/photos/pas_id_51D53D180001_photo-file1.image%252Fjpeg"
> > against the binary data in the incoming item, but rather the content of
> > that file.
> >
> > I probably need to write a for that, right? In that case
> > compare="never" may be the right thing to do again, but I am not sure.
>
> There's a compare="script" for that (might not be in the docs because
> that was added later, if I correctly remember somehow related to
> SyncEvolution...)
I remember vaguely that we discussed it, but I am not using at the
moment and as far as I can tell, also don't need it for PHOTO. I don't
even need a special comparescript. Instead I copy the fields in a
special mergescript.
Here's my field list:
And here's the merge script:
Does that make sense? When would I need a comparescript for PHOTO?
In the script above, the "mode == 1" happens when caching items and
completely overwriting the server side during a slow sync.
Note that this script depends on a few changes and fixes for mergescript
support. In particular, the merge mode must be available and passed
through to MERGEFIELDS().
I broke MERGEFIELDS() when introducing that mode, see attached patch. It
is now still not backward-compatible. Is there a way to have a builtin
function with an optional parameter? If not, then should I introduce a
MERGEFIELDS() without parameter and a MERGEFIELDS_WITH_MODE(mode)?
--
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.
>From c122950f3cca690e68c913eaaa3ae623e294c443 Mon Sep 17 00:00:00 2001
From: Patrick Ohly
Date: Fri, 5 Jul 2013 09:04:15 +0200
Subject: [PATCH 3/4] engine: fix MERGEFIELDS()
Commit de7ff9 introduced a new mode of operation for merging, where
one side gets updated to be identical to the other side, regardless of
what the field list says.
That commit broke MERGEFIELDS() is several ways:
- The extra parameter was not declared in TBuiltInFuncDef
and therefore all attempts to call MERGEFIELDS() from
a script caused a parsing error (something like "closing
bracket expected").
- The parameter was meant to be optional, but not passing it
caused a crash because aFuncContextP->getLocalVar(0) then
is NULL.
This commit fixes these issues, but it still doesn't make the new
parameter optional. That doesn't seem to be possible with the current
TBuiltInFuncDef? Therefore old scripts calling MERGEFIELDS() without
parameter need to be changed to MERGEFIELDS(0).
---
src/sysync/multifielditemtype.cpp |8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/sysync/multifielditemtype.cpp b/src/sysync/multifielditemtype.cpp
index 6dd32d9..da00af4 100755
--- a/src/sysync/multifielditemtype.cpp
+++ b/src/sysync/multifielditemtype.cpp
@@ -195,9 +195,11 @@ public:
static void func_MergeFields(TItemField *&aTermP, TScriptContext *aFuncContextP)
{
TMultiFieldItemType *mfitP = static_cast(aFuncContextP->getCallerContext());
Re: [os-libsynthesis] temporary local ID + FinalizeLocalID
On Wed, 2013-06-26 at 16:10 +0200, Patrick Ohly wrote:
> On Mon, 2013-06-10 at 12:35 +0200, Lukas Zeller wrote:
> > On 10.06.2013, at 11:57, Patrick Ohly wrote:
> > > I'll probably try something else: if commands were delayed in a
> > > message which is marked Final, then force execution of the commands at
> > > the end of processing that message and issue their Status commands, just
> > > as syncing normally would.
> >
> > Sounds like a good compromise to me.
>
> I've implemented that. I've run this through a full test against
> different peers, without problems. Or rather, there were problems
> earlier, which shows that the tests covered relevant corner cases ;-)
> These problems have been addressed.
After adding more tests, I found one more problem: the combination of
and queuing preceding items failed. Patch attached. I intend
to squash it into the "preserve status order" patch before including FDO
master branch.
--
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.
>From 1f01b69ee1aa597924f90c48a299db254a0cd5d9 Mon Sep 17 00:00:00 2001
From: Patrick Ohly
Date: Mon, 1 Jul 2013 08:52:47 +0200
Subject: [PATCH 1/4] merge: ordered status
Add/Replace commands which contain incomplete items are special: they
must trigger a Status 213 response in the same message, which cannot
be done without processing the previously queued commands first.
We could try to queue the incomplete command without processing it at
all, but that would change the response in a way that might be
unexpected by the peer (although legal). Better play it safe and
finish all queued commands, then process the command.
---
src/sysync/synccommand.cpp | 18 ++
src/sysync/synccommand.h |4
src/sysync/syncsession.cpp |7 +++
3 files changed, 29 insertions(+)
diff --git a/src/sysync/synccommand.cpp b/src/sysync/synccommand.cpp
index ef363d1..3b27d63 100755
--- a/src/sysync/synccommand.cpp
+++ b/src/sysync/synccommand.cpp
@@ -2289,6 +2289,24 @@ missingeoc:
} // TSyncOpCommand::AddNextChunk
+// SyncOp commands can execute out of order except when they
+// contain chunked items, because then we would have to issue
+// a 213 Status immediately, which would violate the ordering
+// of Status replies.
+bool TSyncOpCommand::canExecuteOutOfOrder()
+{
+ SmlItemListPtr_t *itemnodePP=&(fSyncOpElementP->itemList);
+ while (*itemnodePP) {
+SmlItemListPtr_t thisitemnode = *itemnodePP;
+if (thisitemnode->item &&
+thisitemnode->item->flags & SmlMoreData_f) {
+ return false;
+}
+itemnodePP = &(thisitemnode->next);
+ }
+ return true;
+}
+
// execute command (perform real actions, generate status)
// returns true if command has executed and can be deleted
bool TSyncOpCommand::execute(void)
diff --git a/src/sysync/synccommand.h b/src/sysync/synccommand.h
index bc5869c..c070905 100755
--- a/src/sysync/synccommand.h
+++ b/src/sysync/synccommand.h
@@ -90,6 +90,9 @@ public:
#endif
// - analyze command (but do not yet execute)
virtual bool analyze(TPackageStates aPackageState) { fPackageState=aPackageState; return true; }; // returns false if command is bad and cannot be executed
+ // - execute() can be called even if there are other already queued commands.
+ // True by default, exceptions must be defined explicitly.
+ virtual bool canExecuteOutOfOrder() { return true; }
// - execute command (perform real actions, generate status)
virtual bool execute(void); // returns true if command could execute, false if it must be queued for later finishing (next message)
// - get number of bytes that will be still available in the workspace after
@@ -413,6 +416,7 @@ public:
virtual ~TSyncOpCommand();
virtual bool isSyncOp() { return true; };
virtual bool analyze(TPackageStates aPackageState);
+ virtual bool canExecuteOutOfOrder();
virtual bool execute(void);
#ifndef USE_SML_EVALUATION
// - get (approximated) message size required for sending it
diff --git a/src/sysync/syncsession.cpp b/src/sysync/syncsession.cpp
index 870342a..a394bed 100644
--- a/src/sysync/syncsession.cpp
+++ b/src/sysync/syncsession.cpp
@@ -2549,6 +2549,13 @@ Ret_t TSyncSession::process(TSmlCommand *aSyncCommandP)
delayExecUntilNextRequest(aSyncCommandP);
}
else {
+if (fDelayedExecutionCommands.size()>0 &&
+!aSyncCommandP->canExecuteOutOfOrder()) {
+ PDEBUGPRINTFX(DBG_SESSION,("%s: command cannot be executed with other commands already delayed -> flush queue",aSyncCommandP->getName()));
+ fCmdIncoming = NULL;
+ tryDelayedExecutionCommands();
+}
+
// command is ok, execute it
fCmdIncomingState=aSyncCommandP->getPackageState();
