Re: Comments on xerces-c patch please

[Svante Signell <svante.signell@telia.com>]

Attached is a more intrusive path, not conditional.
Comments?

On Thu, 2011-08-04 at 15:05 +0200, Svante Signell wrote:
> On Thu, 2011-08-04 at 13:39 +0200, Guillem Jover wrote:
> 
> > Well, you didn't give much time for feedback. Anyway here it goes:
> > 
> > > --- xerces-c-3.1.1/src/xercesc/util/FileManagers/PosixFileMgr.cpp.orig	2008-07-04 09:23:56.000000000 +0000
> > > +++ xerces-c-3.1.1/src/xercesc/util/FileManagers/PosixFileMgr.cpp	2011-08-03 21:58:23.000000000 +0000
> > > @@ -187,6 +187,17 @@
> > >      ArrayJanitor<char> janText(newSrc, manager);
> > >  
> > >      // Use a local buffer that is big enough for the largest legal path
> > > +#ifdef __GNU__
> > > +    char *absPath;
> > > +    XMLCh *ret;
> > > +    // get the absolute path
> > > +    absPath=realpath(newSrc, NULL);
> > > +    if (absPath == NULL)
> > > +   		ThrowXMLwithMemMgr(XMLPlatformUtilsException, XMLExcepts::File_CouldNotGetBasePathName, manager);
> > > +    ret = XMLString::transcode(absPath, manager);
> > > +    free (absPath);
> > > +    return ret; 
> > > +#else
> > 
> > Follow the existing coding style: add a blank line between the
> > variable definintions and the actual code block, add missing spaces
> > around =, remove trailing space after “return ret; ”.
> > 
> > Also given that realpath(path, NULL) is in POSIX.1-2008 and has been
> > supported for a long time on GNU/* and FreeBSD systems I'd change the
> > __GNU__ check to a configure check for the realpath functionality.
> > 
> > >      char absPath[PATH_MAX + 1];
> > >      
> > >      // get the absolute path
> > > @@ -194,20 +205,34 @@
> > >     		ThrowXMLwithMemMgr(XMLPlatformUtilsException, XMLExcepts::File_CouldNotGetBasePathName, manager);
> > >  
> > >      return XMLString::transcode(absPath, manager);
> > > +#endif
> > >  }
> 
> There is already a check for realpath in configure.ac. But using that
> will exclude the else part, see below:
> #else
>     char absPath[PATH_MAX + 1];
> 
>     // get the absolute
> path                                                    
>     if (!realpath(newSrc, absPath))
>                 ThrowXMLwithMemMgr(XMLPlatformUtilsException,
> XMLExcepts::File_\
> CouldNotGetBasePathName, manager);
> 
>     return XMLString::transcode(absPath, manager);
> #endif
> 
> 
> > >  
> > >  XMLCh*
> > >  PosixFileMgr::getCurrentDirectory(MemoryManager* const manager)
> > >  {
> > > +#ifdef __GNU__
> > > +    char *dirBuf=NULL;
> > > +    XMLCh *ret;
> > > +    size_t dummy = 0;
> > > +    char *curDir = getcwd(dirBuf, dummy);
> > > +    if (curDir == NULL)
> > > +        ThrowXMLwithMemMgr(XMLPlatformUtilsException,
> > > +                 XMLExcepts::File_CouldNotGetBasePathName, manager);
> > > +
> > > +    ret = XMLString::transcode(curDir, manager);
> > > +    free (curDir);
> > > +    return ret;
> > > +#else
> > 
> > There's no need for the dummy or dirBuf variables. The same style
> > comments as the above. And getcwd(NULL, 0) has also been supported on
> > FreeBSD and GNU/* for a long time, it would be better to check for its
> > availabilty at configure time and use that whenever possible.
> 
> OK, changed!
> 
> > >      char dirBuf[PATH_MAX + 2];
> > >      char *curDir = getcwd(&dirBuf[0], PATH_MAX + 1);
> > > -
> > 
> > Do not change unrelated spaces and blank lines for other code blocks.
> > 
> > >      if (!curDir)
> > >          ThrowXMLwithMemMgr(XMLPlatformUtilsException,
> > >                   XMLExcepts::File_CouldNotGetBasePathName, manager);
> > >  
> > >      return XMLString::transcode(curDir, manager);
> > > +#endif
> > >  }
> 
> Same here: getcwd is checked for in both if and else parts.
> 
> Should I make it unconditional then? Then probably the DM complains the
> the patch changes the original program code for other archs.
> 
> 
> 


--- xerces-c-3.1.1.modified/src/xercesc/util/FileManagers/PosixFileMgr.cpp.orig	2008-07-04 09:23:56.000000000 +0000
+++ xerces-c-3.1.1.modified/src/xercesc/util/FileManagers/PosixFileMgr.cpp	2011-08-04 14:46:59.000000000 +0000
@@ -187,27 +187,32 @@
     ArrayJanitor<char> janText(newSrc, manager);
 
     // Use a local buffer that is big enough for the largest legal path
-    char absPath[PATH_MAX + 1];
-    
+    char *absPath;
+    XMLCh *ret;
+
     // get the absolute path
-    if (!realpath(newSrc, absPath))
+    absPath = realpath(newSrc, NULL);
+    if (absPath == NULL)
    		ThrowXMLwithMemMgr(XMLPlatformUtilsException, XMLExcepts::File_CouldNotGetBasePathName, manager);
-
-    return XMLString::transcode(absPath, manager);
+    ret = XMLString::transcode(absPath, manager);
+    free(absPath);
+    return ret;
 }
 
 
 XMLCh*
 PosixFileMgr::getCurrentDirectory(MemoryManager* const manager)
 {
-    char dirBuf[PATH_MAX + 2];
-    char *curDir = getcwd(&dirBuf[0], PATH_MAX + 1);
+    XMLCh *ret;
+    char *curDir = getcwd(NULL, 0);
 
-    if (!curDir)
+    if (curDir == NULL)
         ThrowXMLwithMemMgr(XMLPlatformUtilsException,
                  XMLExcepts::File_CouldNotGetBasePathName, manager);
 
-    return XMLString::transcode(curDir, manager);
+    ret = XMLString::transcode(curDir, manager);
+    free(curDir);
+    return ret;
 }