Re: [os-libsynthesis] [PATCH] Improve readability of SyncML TK error messages
Hi Andris,
thanks for the patch. However, the SMLERRPRINTFX() macro, like all debug output
macros across the entire library, is supposed to output a linefeed by itself.
It also actually does so in my builds of libsynthesis, so if the LFs are
missing in your output, it must be something along the path from
SMLERRPRINTFX() to the actual output function.
I think the problem is in the CONSOLEINFO_LIBC shortcut output mode Patrick
added this February (13ff1e4149 (logging + Linux: enable console output)),
where CONSOLEPRINTF() is mapped to fprintf(stderr,...) without adding a LF. In
my (iOS) build this new mode is not used, but output ends in a plain puts(),
which does add the LF.
So my suggestion would be this:
diff --git a/src/sysync/sysync_debug.h b/src/sysync/sysync_debug.h
index 61d4cdd..68dce5e 100755
--- a/src/sysync/sysync_debug.h
+++ b/src/sysync/sysync_debug.h
@@ -110,7 +110,7 @@ TDebugLogger *getDbgLogger(void);
// Because a lot of libs log to stderr, include a unique prefix.
// Assumes that all printf format strings are plain strings.
#define CONSOLEPUTS(m) CONSOLE_PRINTF_VARARGS("%s", (m))
-#define CONSOLE_PRINTF_VARARGS(_m, _args...) fprintf(stderr, "SYSYNC " _m,
##_args)
+#define CONSOLE_PRINTF_VARARGS(_m, _args...) fprintf(stderr, "SYSYNC " _m
"\n", ##_args)
#define CONSOLEPRINTF(m) CONSOLE_PRINTF_VARARGS m
# else // CONSOLEINFO_LIBC
#define CONSOLEPUTS(m) ConsolePuts(m)
Can you please check if this solves the problem in your build?
Lukas
On 05.06.2012, at 14:41, Andris Pavenis wrote:
> Patch attached.
>
> Andris
>
> <0001-Improve-output-of-SyncML-TK-error-messages.patch>___
> os-libsynthesis mailing list
> os-libsynthesis@synthesis.ch
> http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
___
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
Re: [os-libsynthesis] [PATCH] Improve readability of SyncML TK error messages
On 06/07/2012 01:31 PM, Lukas Zeller wrote:
Hi Andris,
thanks for the patch. However, the SMLERRPRINTFX() macro, like all debug output
macros across the entire library, is supposed to output a linefeed by itself.
It also actually does so in my builds of libsynthesis, so if the LFs are
missing in your output, it must be something along the path from
SMLERRPRINTFX() to the actual output function.
I think the problem is in the CONSOLEINFO_LIBC shortcut output mode Patrick
added this February (13ff1e4149 (logging + Linux: enable console output)),
where CONSOLEPRINTF() is mapped to fprintf(stderr,...) without adding a LF. In
my (iOS) build this new mode is not used, but output ends in a plain puts(),
which does add the LF.
So my suggestion would be this:
diff --git a/src/sysync/sysync_debug.h b/src/sysync/sysync_debug.h
index 61d4cdd..68dce5e 100755
--- a/src/sysync/sysync_debug.h
+++ b/src/sysync/sysync_debug.h
@@ -110,7 +110,7 @@ TDebugLogger *getDbgLogger(void);
// Because a lot of libs log to stderr, include a unique prefix.
// Assumes that all printf format strings are plain strings.
#define CONSOLEPUTS(m) CONSOLE_PRINTF_VARARGS("%s", (m))
-#define CONSOLE_PRINTF_VARARGS(_m, _args...) fprintf(stderr, "SYSYNC " _m,
##_args)
+#define CONSOLE_PRINTF_VARARGS(_m, _args...) fprintf(stderr, "SYSYNC " _m
"\n", ##_args)
#define CONSOLEPRINTF(m) CONSOLE_PRINTF_VARARGS m
# else // CONSOLEINFO_LIBC
#define CONSOLEPUTS(m) ConsolePuts(m)
Can you please check if this solves the problem in your build?
If You verify my recent e-mails, you'll find exactly the same suggestion
("\n" added as above).
CONSOLE_PRINTF_VARARGS (and other similar macros) can however be used to
output a message in parts (not only entire message in one piece) in that
case extra "\n" are not needed. The same about the string "SYSYNC "
which should only be present only at the begin of the message but any
more in the middle. That was a reason why I dropped earlier suggestion
which was identical with Yours and suggested other way.
Andris
___
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
Re: [os-libsynthesis] [PATCH] Improve readability of SyncML TK error messages
On 07.06.2012, at 12:43, Andris Pavenis wrote:
> On 06/07/2012 01:31 PM, Lukas Zeller wrote:
>> [...]
>> -#define CONSOLE_PRINTF_VARARGS(_m, _args...) fprintf(stderr, "SYSYNC " _m,
>> ##_args)
>> +#define CONSOLE_PRINTF_VARARGS(_m, _args...) fprintf(stderr, "SYSYNC " _m
>> "\n", ##_args)
>> [...]
>> Can you please check if this solves the problem in your build?
> If You verify my recent e-mails, you'll find exactly the same suggestion
> ("\n" added as above).
Emails to me? Or to the list? I haven't seen anything about that LF topic
before "[os-libsynthesis] [PATCH] Improve readability of SyncML TK error
messages".
> CONSOLE_PRINTF_VARARGS (and other similar macros) can however be used to
> output a message in parts (not only entire message in one piece) in that case
> extra "\n" are not needed.
The convention throughout libsynthesis is that debug macros must output an
entire message, not just a chunk of text, exactly for the reason that the
output channel can do things like prefixing.
If a message line needs to be constructed from parts, that needs to be done
outside of the macro by accumulating into a string variable, and then using the
macro to output it as a whole.
SMLERRPRINTFX() and CONSOLEPRINTF() are no exceptions to this - that's why
these should not be fed strings with trailing LF.
It's the responsibility of the output channel/macro to add an appropriate
end-of-line character. That was missing in the CONSOLE_PRINTF_VARARGS() macro.
> The same about the string "SYSYNC " which should be present only at the begin
> of the message but any more in the middle. That was a reason why I dropped
> earlier suggestion which was identical with Yours and suggested other way.
Agreed, but when following the convention that doesn't happen anyway.
Best Regards,
Lukas
___
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
Re: [os-libsynthesis] [PATCH] Improve readability of SyncML TK error messages
On 06/07/2012 03:24 PM, Lukas Zeller wrote:
On 07.06.2012, at 12:43, Andris Pavenis wrote:
On 06/07/2012 01:31 PM, Lukas Zeller wrote:
[...]
-#define CONSOLE_PRINTF_VARARGS(_m, _args...) fprintf(stderr, "SYSYNC " _m,
##_args)
+#define CONSOLE_PRINTF_VARARGS(_m, _args...) fprintf(stderr, "SYSYNC " _m
"\n", ##_args)
[...]
Can you please check if this solves the problem in your build?
If You verify my recent e-mails, you'll find exactly the same suggestion ("\n"
added as above).
Emails to me? Or to the list? I haven't seen anything about that LF topic before
"[os-libsynthesis] [PATCH] Improve readability of SyncML TK error messages".
It was mentioned together with different topic. The change was identical
so I have tested it.
CONSOLE_PRINTF_VARARGS (and other similar macros) can however be used to output a message
in parts (not only entire message in one piece) in that case extra "\n" are not
needed.
The convention throughout libsynthesis is that debug macros must output an
entire message, not just a chunk of text, exactly for the reason that the
output channel can do things like prefixing.
If a message line needs to be constructed from parts, that needs to be done
outside of the macro by accumulating into a string variable, and then using the
macro to output it as a whole.
SMLERRPRINTFX() and CONSOLEPRINTF() are no exceptions to this - that's why
these should not be fed strings with trailing LF.
It's the responsibility of the output channel/macro to add an appropriate
end-of-line character. That was missing in the CONSOLE_PRINTF_VARARGS() macro.
OK. In that case I agree. Such convention was not self evident
Andris
___
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
Re: [os-libsynthesis] Funambol + refresh-from-server => 417
Hi Patrick,
Just changing getSyncStateAlertCode() call with the second argument set to
false does the same trick. Because that second parameter switches between
"simplified" (true) and "standard conformant" (false) client alert codes
already.
uInt16 alertCode = getSyncStateAlertCode(fServerAlerted,true);
Just because support for refresh-from sync modes was so bad in the early SyncML
days, this parameter remained a constant.
> The "true" part needs to be replaced with a configuration option that
> chooses between old (use "slow sync") and new behavior (use "refresh
> from server").
That now applies to that second parameter of getSyncStateAlertCode()
> Or has enough time passed that we no longer have to care
> about servers which implement slow sync, but not refresh from server?
The benefit of SyncML in general and libsynthesis in particular is that it
works with real legacy systems. In fact, the more time passes, the more the
main reason for using libsynthesis will become smooth legacy support. So I
wouldn't want to switch standard behaviour here, and opt for a config flag.
Best Regards,
Lukas
On 06.06.2012, at 15:49, Patrick Ohly wrote:
> Hello!
>
> It seems that Funambol has implemented some kind of protection against
> excessive slow syncs: I can do a slow sync once fine. Doing it again
> shortly afterwards (say, a few seconds later, as in automated testing)
> results in a 417 "retry later" status for the Alert command (in the
> SyncML response message).
>
> They have not responded to my question about this behavior, so I don't
> have an official confirmation that my interpretation is correct.
>
> This server behavior makes one (otherwise useful) feature of
> libsynthesis a bit troublesome: when an app requests a refresh from
> server ("forceslow" = 1, "syncmode" = 1 in the binfile client API), then
> the engine will ask the server for a slow sync instead of telling it to
> do a refresh-from-server. In other words, the Funambol server has to
> assume the worst (a slow sync) and applies its throttling even though
> the client only wants to do a relatively benign "refresh from server".
>
> I've checked that the Funambol server does many "refresh from server"
> syncs without 417. For that I patched libsynthesis:
>
> diff --git a/src/sysync/localengineds.cpp b/src/sysync/localengineds.cpp
> index ce3d034..98f15dc 100644
> --- a/src/sysync/localengineds.cpp
> +++ b/src/sysync/localengineds.cpp
> @@ -3481,6 +3481,11 @@ localstatus TLocalEngineDS::engGenerateClientSyncAlert(
> ));
> // create appropriate initial alert command
> uInt16 alertCode = getSyncStateAlertCode(fServerAlerted,true);
> + if (alertCode == 201 &&
> + fSyncMode == smo_fromserver &&
> + true) {
> +alertCode = 205;
> + }
> // check for resume
> if (fResuming) {
> // check if what we resume is same as what we wanted to do
>
> Does that patch look right in principle?
>
> The "true" part needs to be replaced with a configuration option that
> chooses between old (use "slow sync") and new behavior (use "refresh
> from server"). Or has enough time passed that we no longer have to care
> about servers which implement slow sync, but not refresh from server?
> Then we could switch to the new behavior unconditionally.
>
> --
> 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.
>
>
>
> ___
> os-libsynthesis mailing list
> os-libsynthesis@synthesis.ch
> http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
___
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
