Re: [Libreoffice-commits] core.git: desktop/source include/LibreOfficeKit

2016-01-19 Thread Oliver Specht 

Hi Miklos,
you're probably right.

Oliver

On 19.01.2016 16:38, Miklos Vajna wrote:

Hi Oliver,

On Tue, Jan 19, 2016 at 06:39:56AM -0800, Oliver Specht  
wrote:

diff --git a/include/LibreOfficeKit/LibreOfficeKit.h 
b/include/LibreOfficeKit/LibreOfficeKit.h
index 7d4210e..8057d75 100644
--- a/include/LibreOfficeKit/LibreOfficeKit.h
+++ b/include/LibreOfficeKit/LibreOfficeKit.h
@@ -51,6 +51,7 @@ struct _LibreOfficeKitClass
   const char* pURL);
  
  char* (*getError) (LibreOfficeKit* pThis);

+void  (*freeError) (const char *pfree);
  
  LibreOfficeKitDocument* (*documentLoadWithOptions) (LibreOfficeKit* pThis,

  const char* pURL,

Won't this break backwards compatibility with existing clients? I think
you can only append new functions to the end of the function list,
otherwise the LIBREOFFICEKIT_HAS_MEMBER() macro is quite pointless. ;-)

Regards,

Miklos


___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source include/LibreOfficeKit

2016-01-19 Thread Stephan Bergmann 

On 01/19/2016 04:38 PM, Miklos Vajna wrote:

On Tue, Jan 19, 2016 at 06:39:56AM -0800, Oliver Specht  
wrote:

diff --git a/include/LibreOfficeKit/LibreOfficeKit.h 
b/include/LibreOfficeKit/LibreOfficeKit.h
index 7d4210e..8057d75 100644
--- a/include/LibreOfficeKit/LibreOfficeKit.h
+++ b/include/LibreOfficeKit/LibreOfficeKit.h
@@ -51,6 +51,7 @@ struct _LibreOfficeKitClass
   const char* pURL);

  char* (*getError) (LibreOfficeKit* pThis);
+void  (*freeError) (const char *pfree);

  LibreOfficeKitDocument* (*documentLoadWithOptions) (LibreOfficeKit* pThis,
  const char* pURL,


Won't this break backwards compatibility with existing clients? I think
you can only append new functions to the end of the function list,
otherwise the LIBREOFFICEKIT_HAS_MEMBER() macro is quite pointless. ;-)


...and the usage of char* vs. char const* seems curiously inconsistent 
in that API


___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source include/LibreOfficeKit

2016-01-19 Thread Miklos Vajna 
Hi Oliver,

On Tue, Jan 19, 2016 at 06:39:56AM -0800, Oliver Specht  
wrote:
> diff --git a/include/LibreOfficeKit/LibreOfficeKit.h 
> b/include/LibreOfficeKit/LibreOfficeKit.h
> index 7d4210e..8057d75 100644
> --- a/include/LibreOfficeKit/LibreOfficeKit.h
> +++ b/include/LibreOfficeKit/LibreOfficeKit.h
> @@ -51,6 +51,7 @@ struct _LibreOfficeKitClass
>   const char* pURL);
>  
>  char* (*getError) (LibreOfficeKit* pThis);
> +void  (*freeError) (const char *pfree);
>  
>  LibreOfficeKitDocument* (*documentLoadWithOptions) (LibreOfficeKit* 
> pThis,
>  const char* pURL,

Won't this break backwards compatibility with existing clients? I think
you can only append new functions to the end of the function list,
otherwise the LIBREOFFICEKIT_HAS_MEMBER() macro is quite pointless. ;-)

Regards,

Miklos


signature.asc
Description: Digital signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2015-07-06 Thread Stephan Bergmann 
I would just not go down the troubled road of attempting to support 
whatever platform-specific file pathname notation in an interface 
supposedly designed to take URLs (i.e., stop calling 
osl::FileBase::getFileURLFromSystemPath on aURL), and then---if there is 
indeed demand to support relative URLs in your interface---use 
rtl::Uri::convertRelToAbs for reliable conversion, instead of resorting 
to guesswork like indexOf(://).


On 07/03/2015 06:18 PM, Jan Holesovsky wrote:

commit e83cb37cf7546e8bc46d0d49b487dcd352b67093
Author: Jan Holesovsky ke...@collabora.com
Date:   Fri Jul 3 18:14:31 2015 +0200

 LOK: Don't try to absolutize URL's.

 Based on a patch by Henry Castro.

 Change-Id: Ia7aca20feb8f6095adf7dfe510ed78b1e9882740

diff --git a/desktop/source/lib/init.cxx b/desktop/source/lib/init.cxx
index ee47cd8..3a0ce67 100644
--- a/desktop/source/lib/init.cxx
+++ b/desktop/source/lib/init.cxx
@@ -161,13 +161,18 @@ static OUString getUString(const char* pString)
  return OStringToOUString(sString, RTL_TEXTENCODING_UTF8);
  }

-// Try to convert a relative URL to an absolute one
+/// Try to convert a relative URL to an absolute one, unless it already looks 
like an URL.
  static OUString getAbsoluteURL(const char* pURL)
  {
-OUString aURL( getUString( pURL ) );
+OUString aURL(getUString(pURL));
+
+// return unchanged if it likely is an URL already
+if (aURL.indexOf(://)  0)
+return aURL;
+
  OUString sAbsoluteDocUrl, sWorkingDir, sDocPathUrl;

-// FIXME: this would appear to kill non-file URLs.
+// convert relative paths to absolute ones
  osl_getProcessWorkingDir(sWorkingDir.pData);
  osl::FileBase::getFileURLFromSystemPath( aURL, sDocPathUrl );
  osl::FileBase::getAbsoluteFileURL(sWorkingDir, sDocPathUrl, 
sAbsoluteDocUrl);
@@ -343,7 +348,7 @@ static LibreOfficeKitDocument* 
lo_documentLoadWithOptions(LibreOfficeKit* pThis,

  SolarMutexGuard aGuard;

-OUString aURL = getAbsoluteURL(pURL);
+OUString aURL(getAbsoluteURL(pURL));

  pLib-maLastExceptionMsg.clear();

@@ -409,7 +414,7 @@ static int doc_saveAs(LibreOfficeKitDocument* pThis, const 
char* sUrl, const cha
  LibLODocument_Impl* pDocument = static_castLibLODocument_Impl*(pThis);

  OUString sFormat = getUString(pFormat);
-OUString aURL = getAbsoluteURL(sUrl);
+OUString aURL(getAbsoluteURL(sUrl));

  try
  {

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2015-07-06 Thread Jan Holesovsky 
Hi Stephan,

Stephan Bergmann píše v Po 06. 07. 2015 v 12:06 +0200:

 I would just not go down the troubled road of attempting to support 
 whatever platform-specific file pathname notation in an interface 
 supposedly designed to take URLs (i.e., stop calling 
 osl::FileBase::getFileURLFromSystemPath on aURL), and then---if there is 
 indeed demand to support relative URLs in your interface---use 
 rtl::Uri::convertRelToAbs for reliable conversion, instead of resorting 
 to guesswork like indexOf(://).

Thanks for the hint! :-)  Pushed, 5.0 cherry-pick request here:

https://gerrit.libreoffice.org/16795 

All the best,
Kendy

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2013-02-21 Thread Stephan Bergmann 

On 02/20/2013 04:25 PM, julien2412 wrote:

Lubos Lunak wrote

Lubos Lunak wrote

  Sorry, I did not realize that = { 0 } actually clears the rest of the
array.
And I do not quite understand why clearing an entire array is supposed to
be
a good way to initialize it when just setting the last one to 0 is enough.
...

Why pReceiveBuffer[nBytes-1] = 0; would need to stay?


  If I'm reading the code correctly, nBytes-1 is not the position after
the
read data, but the last read item. Which should mean that the repeated
recv()
may rewrite all the 0's from the initialization. That rather begs the
question why it changes the last read item instead of terminating it, so
the
code may have been already broken to begin with.


I thought it could be interesting to put together declaration and
initialization so we're sure we won't have some side effects (I thought it
could be more clean too).
To be honest, I thought too it was a very straightforward patch, so i pushed
it on master instead of having submitted for gerrit review. Obviously, I was
wrong.

For the second part of your message, I really don't know.

Again, don't hesitate to tell me if I must revert this commit.


FYI:  Without having seen this mail thread, I did see the couple of 
recent commits to desktop/source/app/officeipcthread.cxx last night, saw 
that they still don't address all problems with the sending side not 
including NUL bytes at places where the receiving side expects them, 
worked on a clean-up of that code, but didn't come around yet to push 
it.  Will happen shortly.


Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2013-02-21 Thread Lubos Lunak 
On Thursday 21 of February 2013, Stephan Bergmann wrote:
 FYI:  Without having seen this mail thread, I did see the couple of
 recent commits to desktop/source/app/officeipcthread.cxx last night, saw
 that they still don't address all problems with the sending side not
 including NUL bytes at places where the receiving side expects them,
 worked on a clean-up of that code, but didn't come around yet to push
 it.  Will happen shortly.

 I don't know what the code is supposed to do exactly, but just from looking 
at it I agree with Matteo's comments on it.

-- 
 Lubos Lunak
 l.lu...@suse.cz
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2013-02-21 Thread Norbert Thiebaud 
On Wed, Feb 20, 2013 at 7:54 AM, julien2412 serval2...@yahoo.fr wrote:

 Yes I meant it, why? Is it wrong?
 if pReceiveBuffer is initialized with 0 for the (sc_nCSASeqLength + 1)
 elements thanks to = {0} initialization, what obvious thing did I miss? Why
 pReceiveBuffer[nBytes-1] = 0; would need to stay?

Ok since I added that particular code,  let me explain

 -if (nBytes  0)
 -{
 -pReceiveBuffer[nBytes-1] = 0;
 -}

was needed because the loop before can exit under 2 conditions:
the normal one where  a[nBytes - 1] is already 0
or the 'non-normal' one when the recv return 0 (or -1 for that matter)
which mean the pipe got interrupted prematurely, in which case the
buffer would _not_ be 0-terminated
which was relyied upon later.

there is also the case where recv bail out before reading anything at
that point nByte = 0 si of course we do not want to do a[nByte - 1]
and for that case a[0] is set to 0;

initializing the whole buffer is a waste in all case but the corner
case where the pipe get broken.

Note: it is clearly not expected that the pipe close with receiving
a\0 as last character... ortherwise we would already be segfaulting a
lot due to the code aftert that that _assume_ that the buffer is a
0-terminated string.
so there is no particular danger that a valid character would be
overwritten in normal expected cases.

Norbert
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2013-02-21 Thread julien2412 
Norbert Thiebaud wrote
 On Wed, Feb 20, 2013 at 7:54 AM, julien2412 lt;

 serval2412@

 gt; wrote:

 Yes I meant it, why? Is it wrong?
 if pReceiveBuffer is initialized with 0 for the (sc_nCSASeqLength + 1)
 elements thanks to = {0} initialization, what obvious thing did I miss?
 Why
 pReceiveBuffer[nBytes-1] = 0; would need to stay?
 
 Ok since I added that particular code,  let me explain
 
 -if (nBytes  0)
 -{
 -pReceiveBuffer[nBytes-1] = 0;
 -}
 
 was needed because the loop before can exit under 2 conditions:
 the normal one where  a[nBytes - 1] is already 0
 or the 'non-normal' one when the recv return 0 (or -1 for that matter)
 which mean the pipe got interrupted prematurely, in which case the
 buffer would _not_ be 0-terminated
 which was relyied upon later.
 
 there is also the case where recv bail out before reading anything at
 that point nByte = 0 si of course we do not want to do a[nByte - 1]
 and for that case a[0] is set to 0;
 
 initializing the whole buffer is a waste in all case but the corner
 case where the pipe get broken.
 
 Note: it is clearly not expected that the pipe close with receiving
 a\0 as last character... ortherwise we would already be segfaulting a
 lot due to the code aftert that that _assume_ that the buffer is a
 0-terminated string.
 so there is no particular danger that a valid character would be
 overwritten in normal expected cases.

Thank you Norbert for the explanation.
As I indicated in my previous message, I would have been ready to revert my
commit but it seems Stephan fixed the whole thing, see
https://gerrit.libreoffice.org/gitweb?p=core.git;a=commitdiff;h=b6ff19fba3a1a6d65134539a71d7a8df0e35d4ff.

Julien
PS : I thought I had found something useful but I must definitely forget
about {0} initialization since it must be used far more carefully than I
expected :-)



--
View this message in context: 
http://nabble.documentfoundation.org/Re-Libreoffice-commits-core-git-desktop-source-tp4038892p4039132.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2013-02-20 Thread Lubos Lunak 
On Tuesday 19 of February 2013, Julien Nabet wrote:
  desktop/source/app/officeipcthread.cxx |8 +---
  1 file changed, 1 insertion(+), 7 deletions(-)

 New commits:
 commit 7d9a7020eb5777f5baaa8beb6af5db9a8796c7c9
 Author: Julien Nabet serval2...@yahoo.fr
 Date:   Tue Feb 19 21:35:19 2013 +0100

 Good way to initialize array of char char var[NB]={0}

 See http://stackoverflow.com/questions/1920430/c-array-initialization

 Change-Id: Ibbbe249684dc34f8aa44868c99cc1344a2928ade

 diff --git a/desktop/source/app/officeipcthread.cxx
 b/desktop/source/app/officeipcthread.cxx index 8db7946..445ccb4 100644
 --- a/desktop/source/app/officeipcthread.cxx
 +++ b/desktop/source/app/officeipcthread.cxx
 @@ -497,23 +497,17 @@ OfficeIPCThread::Status
 OfficeIPCThread::EnableOfficeIPCThread() else if( pThread-maPipe.create(
 aPipeIdent.getStr(), osl_Pipe_OPEN, rSecurity )) // Creation not
 successfull, now we try to connect {
  osl::StreamPipe aStreamPipe(pThread-maPipe.getHandle());
 -char pReceiveBuffer[sc_nCSASeqLength + 1];
 +char pReceiveBuffer[sc_nCSASeqLength + 1] = {0};
  int nResult = 0;
  int nBytes = 0;
  int nBufSz = sc_nCSASeqLength + 1;
  // read byte per byte
 -pReceiveBuffer[0] = 0;
  while ((nResult=aStreamPipe.recv( pReceiveBuffer+nBytes,
 nBufSz-nBytes))0) { nBytes += nResult;
  if (pReceiveBuffer[nBytes-1]=='\0') {
  break;
  }
  }
 -/* make sure the buffer is \0 terminated */
 -if (nBytes  0)
 -{
 -pReceiveBuffer[nBytes-1] = 0;
 -}

 Did you really mean to remove this part?

-- 
 Lubos Lunak
 l.lu...@suse.cz
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2013-02-20 Thread julien2412 
Lubos Lunak wrote
 On Tuesday 19 of February 2013, Julien Nabet wrote:
 diff --git a/desktop/source/app/officeipcthread.cxx
 b/desktop/source/app/officeipcthread.cxx index 8db7946..445ccb4 100644
 --- a/desktop/source/app/officeipcthread.cxx
 +++ b/desktop/source/app/officeipcthread.cxx
 @@ -497,23 +497,17 @@ OfficeIPCThread::Status
 OfficeIPCThread::EnableOfficeIPCThread() else if( pThread-maPipe.create(
 aPipeIdent.getStr(), osl_Pipe_OPEN, rSecurity )) // Creation not
 successfull, now we try to connect {
  osl::StreamPipe aStreamPipe(pThread-maPipe.getHandle());
 -char pReceiveBuffer[sc_nCSASeqLength + 1];
 +char pReceiveBuffer[sc_nCSASeqLength + 1] = {0};
  int nResult = 0;
  int nBytes = 0;
  int nBufSz = sc_nCSASeqLength + 1;
  // read byte per byte
 -pReceiveBuffer[0] = 0;
  while ((nResult=aStreamPipe.recv( pReceiveBuffer+nBytes,
 nBufSz-nBytes))0) { nBytes += nResult;
  if (pReceiveBuffer[nBytes-1]=='\0') {
  break;
  }
  }
 -/* make sure the buffer is \0 terminated */
 -if (nBytes  0)
 -{
 -pReceiveBuffer[nBytes-1] = 0;
 -}
  Did you really mean to remove this part?

Hi Lubos,

Yes I meant it, why? Is it wrong?
if pReceiveBuffer is initialized with 0 for the (sc_nCSASeqLength + 1)
elements thanks to = {0} initialization, what obvious thing did I miss? Why
pReceiveBuffer[nBytes-1] = 0; would need to stay?

Of course, if my commit is wrong, I'll revert it after my day time job

Julien



--
View this message in context: 
http://nabble.documentfoundation.org/Re-Libreoffice-commits-core-git-desktop-source-tp4038892p4038893.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2013-02-20 Thread Matteo Casalin 
Hi all,
I may be completely wrong and I really don't know this code and what it is 
aimet to, but I'm worried that

if (nBytes  0)
{
pReceiveBuffer[nBytes-1] = 0;
}

overwrites a received character (that could be '\0' or not).

If I understand correctly, the target is to have the sequence of characters 
being '\0' terminated whether that '\0' whas in the stream or not, and then I 
would say that the whole routine should be something like:

char pReceiveBuffer[sc_nCSASeqLength + 1] = {0};
int nResult = 0;
int nBytes = 0;
// read byte per byte
while ((nResult=aStreamPipe.recv( pReceiveBuffer+nBytes, 
sc_nCSASeqLength-nBytes))0)
{
nBytes += nResult;
if (pReceiveBuffer[nBytes-1]=='\0')
{
break;
}
}
// unconditionally end stream with '\0'
pReceiveBuffer[nBytes] = '\0';

In case a '\0' is already in the sequence and the additional '\0' is undesired, 
nBytes can be adjusted in the if (pReceiveBuffer[nBytes-1]=='\0') so to 
overwrite the existing one.
Also, the read byte per byte comment looks wrong to me.

If I misunderstood the point then apologies for the noise.
Cheers

Matteo



On Wed, 20 Feb 2013 05:54:50 -0800 (PST)
julien2412 serval2...@yahoo.fr wrote:

 Lubos Lunak wrote
  On Tuesday 19 of February 2013, Julien Nabet wrote:
  diff --git a/desktop/source/app/officeipcthread.cxx
  b/desktop/source/app/officeipcthread.cxx index 8db7946..445ccb4 100644
  --- a/desktop/source/app/officeipcthread.cxx
  +++ b/desktop/source/app/officeipcthread.cxx
  @@ -497,23 +497,17 @@ OfficeIPCThread::Status
  OfficeIPCThread::EnableOfficeIPCThread() else if( pThread-maPipe.create(
  aPipeIdent.getStr(), osl_Pipe_OPEN, rSecurity )) // Creation not
  successfull, now we try to connect {
   osl::StreamPipe aStreamPipe(pThread-maPipe.getHandle());
  -char pReceiveBuffer[sc_nCSASeqLength + 1];
  +char pReceiveBuffer[sc_nCSASeqLength + 1] = {0};
   int nResult = 0;
   int nBytes = 0;
   int nBufSz = sc_nCSASeqLength + 1;
   // read byte per byte
  -pReceiveBuffer[0] = 0;
   while ((nResult=aStreamPipe.recv( pReceiveBuffer+nBytes,
  nBufSz-nBytes))0) { nBytes += nResult;
   if (pReceiveBuffer[nBytes-1]=='\0') {
   break;
   }
   }
  -/* make sure the buffer is \0 terminated */
  -if (nBytes  0)
  -{
  -pReceiveBuffer[nBytes-1] = 0;
  -}
   Did you really mean to remove this part?
 
 Hi Lubos,
 
 Yes I meant it, why? Is it wrong?
 if pReceiveBuffer is initialized with 0 for the (sc_nCSASeqLength + 1)
 elements thanks to = {0} initialization, what obvious thing did I miss? Why
 pReceiveBuffer[nBytes-1] = 0; would need to stay?
 
 Of course, if my commit is wrong, I'll revert it after my day time job
 
 Julien
 
 
 
 --
 View this message in context: 
 http://nabble.documentfoundation.org/Re-Libreoffice-commits-core-git-desktop-source-tp4038892p4038893.html
 Sent from the Dev mailing list archive at Nabble.com.
 ___
 LibreOffice mailing list
 LibreOffice@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/libreoffice
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2013-02-20 Thread Matteo Casalin 
On Wed, 20 Feb 2013 15:32:10 +0100
Matteo Casalin matteo.casa...@gmx.com wrote:

 Hi all,
 I may be completely wrong and I really don't know this code and what it 
 is aimet to, but I'm worried that
 
 if (nBytes  0)
 {
 pReceiveBuffer[nBytes-1] = 0;
 }
 
 overwrites a received character (that could be '\0' or not).
 
 If I understand correctly, the target is to have the sequence of characters 
 being '\0' terminated whether that '\0' whas in the stream or not, and then I 
 would say that the whole routine should be something like:
 
 char pReceiveBuffer[sc_nCSASeqLength + 1] = {0};

As an additional thought, this could then be :

  char pReceiveBuffer[sc_nCSASeqLength + 1];

 int nResult = 0;
 int nBytes = 0;
 // read byte per byte
 while ((nResult=aStreamPipe.recv( pReceiveBuffer+nBytes, 
 sc_nCSASeqLength-nBytes))0)
 {
 nBytes += nResult;
 if (pReceiveBuffer[nBytes-1]=='\0')
 {
 break;
 }
 }
 // unconditionally end stream with '\0'
 pReceiveBuffer[nBytes] = '\0';
 
 In case a '\0' is already in the sequence and the additional '\0' is 
 undesired, nBytes can be adjusted in the if 
 (pReceiveBuffer[nBytes-1]=='\0') so to overwrite the existing one.
 Also, the read byte per byte comment looks wrong to me.
 
 If I misunderstood the point then apologies for the noise.
 Cheers
 
 Matteo
 
 
 
 On Wed, 20 Feb 2013 05:54:50 -0800 (PST)
 julien2412 serval2...@yahoo.fr wrote:
 
  Lubos Lunak wrote
   On Tuesday 19 of February 2013, Julien Nabet wrote:
   diff --git a/desktop/source/app/officeipcthread.cxx
   b/desktop/source/app/officeipcthread.cxx index 8db7946..445ccb4 100644
   --- a/desktop/source/app/officeipcthread.cxx
   +++ b/desktop/source/app/officeipcthread.cxx
   @@ -497,23 +497,17 @@ OfficeIPCThread::Status
   OfficeIPCThread::EnableOfficeIPCThread() else if( pThread-maPipe.create(
   aPipeIdent.getStr(), osl_Pipe_OPEN, rSecurity )) // Creation not
   successfull, now we try to connect {
osl::StreamPipe aStreamPipe(pThread-maPipe.getHandle());
   -char pReceiveBuffer[sc_nCSASeqLength + 1];
   +char pReceiveBuffer[sc_nCSASeqLength + 1] = {0};
int nResult = 0;
int nBytes = 0;
int nBufSz = sc_nCSASeqLength + 1;
// read byte per byte
   -pReceiveBuffer[0] = 0;
while ((nResult=aStreamPipe.recv( pReceiveBuffer+nBytes,
   nBufSz-nBytes))0) { nBytes += nResult;
if (pReceiveBuffer[nBytes-1]=='\0') {
break;
}
}
   -/* make sure the buffer is \0 terminated */
   -if (nBytes  0)
   -{
   -pReceiveBuffer[nBytes-1] = 0;
   -}
Did you really mean to remove this part?
  
  Hi Lubos,
  
  Yes I meant it, why? Is it wrong?
  if pReceiveBuffer is initialized with 0 for the (sc_nCSASeqLength + 1)
  elements thanks to = {0} initialization, what obvious thing did I miss? Why
  pReceiveBuffer[nBytes-1] = 0; would need to stay?
  
  Of course, if my commit is wrong, I'll revert it after my day time job
  
  Julien
  
  
  
  --
  View this message in context: 
  http://nabble.documentfoundation.org/Re-Libreoffice-commits-core-git-desktop-source-tp4038892p4038893.html
  Sent from the Dev mailing list archive at Nabble.com.
  ___
  LibreOffice mailing list
  LibreOffice@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/libreoffice
 ___
 LibreOffice mailing list
 LibreOffice@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/libreoffice
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2013-02-20 Thread Lubos Lunak 
On Wednesday 20 of February 2013, julien2412 [via Document Foundation Mail 
Archive] wrote:
 Lubos Lunak wrote

  On Tuesday 19 of February 2013, Julien Nabet wrote:
  diff --git a/desktop/source/app/officeipcthread.cxx
  b/desktop/source/app/officeipcthread.cxx index 8db7946..445ccb4 100644
  --- a/desktop/source/app/officeipcthread.cxx
  +++ b/desktop/source/app/officeipcthread.cxx
  @@ -497,23 +497,17 @@ OfficeIPCThread::Status
  OfficeIPCThread::EnableOfficeIPCThread() else if(
  pThread-maPipe.create( aPipeIdent.getStr(), osl_Pipe_OPEN, rSecurity ))
  // Creation not successfull, now we try to connect {
   osl::StreamPipe aStreamPipe(pThread-maPipe.getHandle());
  -char pReceiveBuffer[sc_nCSASeqLength + 1];
  +char pReceiveBuffer[sc_nCSASeqLength + 1] = {0};
   int nResult = 0;
   int nBytes = 0;
   int nBufSz = sc_nCSASeqLength + 1;
   // read byte per byte
  -pReceiveBuffer[0] = 0;
   while ((nResult=aStreamPipe.recv( pReceiveBuffer+nBytes,
  nBufSz-nBytes))0) { nBytes += nResult;
   if (pReceiveBuffer[nBytes-1]=='\0') {
   break;
   }
   }
  -/* make sure the buffer is \0 terminated */
  -if (nBytes  0)
  -{
  -pReceiveBuffer[nBytes-1] = 0;
  -}
 
   Did you really mean to remove this part?

 Hi Lubos,

 Yes I meant it, why? Is it wrong?
 if pReceiveBuffer is initialized with 0 for the (sc_nCSASeqLength + 1)
 elements thanks to = {0} initialization, what obvious thing did I miss?

 Sorry, I did not realize that = { 0 } actually clears the rest of the array. 
And I do not quite understand why clearing an entire array is supposed to be 
a good way to initialize it when just setting the last one to 0 is enough.

 Why pReceiveBuffer[nBytes-1] = 0; would need to stay?

 If I'm reading the code correctly, nBytes-1 is not the position after the 
read data, but the last read item. Which should mean that the repeated recv() 
may rewrite all the 0's from the initialization. That rather begs the 
question why it changes the last read item instead of terminating it, so the 
code may have been already broken to begin with.

-- 
 Lubos Lunak
 l.lu...@suse.cz




--
View this message in context: 
http://nabble.documentfoundation.org/Re-Libreoffice-commits-core-git-desktop-source-tp4038892p4038899.html
Sent from the Dev mailing list archive at Nabble.com.___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice-commits] core.git: desktop/source

2013-02-20 Thread julien2412 
Lubos Lunak wrote
 Lubos Lunak wrote
  Sorry, I did not realize that = { 0 } actually clears the rest of the
 array. 
 And I do not quite understand why clearing an entire array is supposed to
 be 
 a good way to initialize it when just setting the last one to 0 is enough.
 ...
 Why pReceiveBuffer[nBytes-1] = 0; would need to stay?
 
  If I'm reading the code correctly, nBytes-1 is not the position after
 the 
 read data, but the last read item. Which should mean that the repeated
 recv() 
 may rewrite all the 0's from the initialization. That rather begs the 
 question why it changes the last read item instead of terminating it, so
 the 
 code may have been already broken to begin with.

I thought it could be interesting to put together declaration and
initialization so we're sure we won't have some side effects (I thought it
could be more clean too).
To be honest, I thought too it was a very straightforward patch, so i pushed
it on master instead of having submitted for gerrit review. Obviously, I was
wrong.

For the second part of your message, I really don't know.

Again, don't hesitate to tell me if I must revert this commit.

Julien



--
View this message in context: 
http://nabble.documentfoundation.org/Re-Libreoffice-commits-core-git-desktop-source-tp4038892p4038909.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice