As stated by a number of comments on bug 7619, the message terminator for SIP2 messages should be configurable as neither CR ( the standard terminator ) nor CRLF ( the more compatible terminator ) give full compatibility for all SIP2 devices.
Created attachment 15014 [details] [review] Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml Add a delimeter option to SIPConfig.xml, choices for 'delimeter' are 'CR' or 'CRLF'. The default continues to be 'CRLF' if 'delimeter' is undefined. Test Plan: 1) Apply patch 2) Start SIP server 3) Run C4/SIP/t/04patron_status.t 4) Stop SIP server 5) Add terminator="CR" for account login 'term1' 6) Run 04patron_status.t again, you should see no change
I'm upgrading this to a bug, as the existing behavior causes incompatibility with many SIP devices and services ( such as Overdrive ).
Created attachment 15204 [details] [review] Signed-off patch Make sense. With this patch, starting 2 SIP2 servers, it is possible to serve heterogeneous Self check devices. Shouldn't we need some doc somewhere about this parameter?
Frédéric, since the behavior is controlled at the "accounts" level, 1 SIP server instance can handle both styles! Yes, I think I will alert Nicole to add this to the manual.
(En réponse au commentaire 4) > Frédéric, since the behavior is controlled at the "accounts" level, 1 SIP > server instance can handle both styles! Yes, I think I will alert Nicole to > add this to the manual. Yes, even better. Whatever am I thinking of?
Tested OK. No big deal but I'm wondering: if the default mode is still CRLF when the parameter is not set, shouldn't we set the default line in SIPconfig.xml to CRLF instead of CR?
I was under the assumption that the line I altered was an example. I can post a followup to change that if you wish. (In reply to comment #6) > Tested OK. > No big deal but I'm wondering: if the default mode is still CRLF when the > parameter is not set, shouldn't we set the default line in SIPconfig.xml to > CRLF instead of CR?
Hi Kyle, could you perhaps provide a little follow up for the tabulation characters? * C4/SIP/Sip/MsgType.pm FAIL pod OK forbidden patterns FAIL The patch introduces a forbidden pattern: tabulation character (869) The patch introduces a forbidden pattern: tabulation character (793) The patch introduces a forbidden pattern: tabulation character (781) valid OK critic OK Apart from this all tests and the QA script pass. SIP is hard to test and I am still not familiar with it. Therefore I would like to call for a second sign off on this and then it can pass QA.
Created attachment 18193 [details] [review] Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml Add a delimeter option to SIPConfig.xml, choices for 'delimeter' are 'CR' or 'CRLF'. The default continues to be 'CRLF' if 'delimeter' is undefined. Test Plan: 1) Apply patch 2) Start SIP server 3) Run C4/SIP/t/04patron_status.t 4) Stop SIP server 5) Add terminator="CR" for account login 'term1' 6) Run 04patron_status.t again, you should see no change Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Make sense. With this patch, starting 2 SIP2 servers, it is possible to serve heterogeneous Self check devices. Shouldn't we need some doc somewhere about this parameter?
Created attachment 18267 [details] [review] Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml Signed off. With my own SIP testing scripts, I get good results (ie. no change) with CRLF.
Created attachment 18323 [details] [review] Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml Add a delimeter option to SIPConfig.xml, choices for 'delimeter' are 'CR' or 'CRLF'. The default continues to be 'CRLF' if 'delimeter' is undefined. Test Plan: 1) Apply patch 2) Start SIP server 3) Run C4/SIP/t/04patron_status.t 4) Stop SIP server 5) Add terminator="CR" for account login 'term1' 6) Run 04patron_status.t again, you should see no change Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Signed-off-by: Adrien Saurat <adrien.saurat@biblibre.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Pushed to master. Thanks, Kyle!
Tomas, Ive checked this is ok on 3.12.x too. So if you want to push it to that, it should be fine
This patch has been pushed to 3.12.x, will be in 3.12.7. Thanks Kyle!
This patch has been pushed to 3.8.x, and will be in 3.08.20.