#769 Unix : relative path not supported

v4.0
closed
jfaucher
5
2012-08-14
2009-07-19
jfaucher
No

This code prints the error message whereas the file exists and the relative path is good :
stream = .stream~new("../datas/file.txt")
if stream~query("exists") == "" then do
say "File "filename" not found"
exit 1
end

The problems comes from SysFileSystem::qualifyStreamName
where the case '.' is not good.
If I remove this case to reach the default, then it's ok.

I will do the fix, if you agree. I tested with these paths, it's ok :
"../datas/file.txt"
"../file.txt"
"./file.txt"
".file.txt"
I don't think I forget a special case for Linux ?

Jean-Louis

Discussion

  • Mark Miesfeld
    Mark Miesfeld
    2009-07-19

    I don't have a Linux system up right now, but looking at the code, you change seems okay, maybe. <grin>

    What happens if you do this:

    f = .stream~new('../datas/file.txt')
    say 'f:' f~qualify

    Do you get the right fully qualified path?

    Can you add a test case that would catch this bug also?

    --
    Mark Miesfeld

     
  • Mark Miesfeld
    Mark Miesfeld
    2009-07-19

    Jean-Louis,

    Plus, I'd say just go ahead and do the fix. If Rick thinks there is a better way, he's not shy about changing it.

     
  • jfaucher
    jfaucher
    2009-07-19

    Mark,

    ok, I will commit the fix and add a test case.
    I have this result for your test :
    f: /mnt/local/ooRexx/profiling/csv/stream/../datas/file.txt
    where
    pwd = /mnt/local/ooRexx/profiling/csv/stream

    This path is well supported, I can read the file without problem.

     
  • Mark Miesfeld
    Mark Miesfeld
    2009-07-19

    My personal feeling is that f~qualify should return:

    /mnt/local/ooRexx/profiling/csv/stream/datas/file.txt

    not

    /mnt/local/ooRexx/profiling/csv/stream/../datas/file.txt

    On Windows there are functions to remove the unneeded ..\ from a fully qualified path. Not sure what unix functions there are to do the same thing. But it should be done. You could test your output under 3.2.0 and see what you get. I bet you will get:

    /mnt/local/ooRexx/profiling/csv/stream/datas/file.txt

    and if so, then it needs to match that under 4.0.0

    --
    Mark Miesfeld

     
  • Mark Miesfeld
    Mark Miesfeld
    2009-07-19

    Jean-Louis,

    I just checked 3.2.0 on a Linux and my small test program returns:

    /mnt/local/ooRexx/profiling/csv/stream/datas/file.txt

    not

    /mnt/local/ooRexx/profiling/csv/stream/../datas/file.txt

    So that needs to be fixed also.

     
  • jfaucher
    jfaucher
    2009-07-19

    It seems we have canonicalizeName which could do that.
    I will have a look at that...
    By the way, I see the same kind of problem in SysFileSystem::searchFileName : case '.'
    That becomes a little bit hot because the impact becomes wider than expected.

    Jean-Louis

     
  • Mark Miesfeld
    Mark Miesfeld
    2009-07-19

    Yeah, I thought the impact was wider than it first appeared.

    One approach would be to write several tests cases to test this aspect throughly, as many as needed. Then fix the code so that they all passed.

     
  • jfaucher
    jfaucher
    2009-07-19

    Mark,

    I committed a new fix, in trunk only.
    Seems to work as expected.
    Problem : I won't have time to write test cases now, I will do that probably next WE.

    I leave you decide when to commit the fix in 4.0.0 (if applicable).

    Jean-Louis

     
  • Mark Miesfeld
    Mark Miesfeld
    2009-07-21

    Jean-Louis

    I added a number of test cases using relative path names for qualify and query exists.

    On Windows, with trunk, the entire test suite runs without errors. Unfortunately, on Linux the entire test suite has 162 failures and 11 errors.

    I just started looking at it, but a first guess is that the interpreter is not locating files. For instance, this error:

    [error] [20090720 17:00:13.511169]
    svn: r3371 Change date: 2008-09-20 21:33:29 -0700
    Test: TEST_RESULT_WITH_RETURN_EXTERNALFILE
    Class: RESULT_RC_SIGL.testGroup
    File: /home/miesfeld/.../base/special.variables/RESULT_RC_SIGL.testGroup
    Event: [SYNTAX 43.1] raised unexpectedly.
    Could not find routine "ooRexx.Base.RESULT.rex"
    Line: 199
    199 - call (fileName) -- call external program

    comes from this test case:

    ::method "test_Result_with_Return_ExternalFile"
    expose fileName
    call (fileName) -- call external program

    if var("RESULT") then tmpResult=result -- save value of "result"
    if var("RC") then tmpRC =rc -- save value of "rc"

    fileName is set in the test case here:

    ::method init
    expose fileName
    fileName="ooRexx.Base.RESULT.rex" -- define temporary file name
    forward class (super)

    So, we'll have to take a closer look at the patch. I glanced through it and thought it seemed okay, but ...

     
  • Mark Miesfeld
    Mark Miesfeld
    2009-07-21

    It turns out the behaviour of realpath() is a little quirky, from my point of view. It will canonicalize the path name, but then return NULL and set errno to 2, ENOENT, if the named file does not exist. The print out below shows this.

    So, for instance, on doing a stream open to write a new file, the interpreter is now failing. Which is why so many test cases are failing.

    Committed revision 4967.

    With that commit, the entire test suite runs without any failures on Fedora Core 10 32-bit (trunk.)

    Not sure if this is the final fix or not, but at least it works with the test suite. <grin>

    / qTest.rex /
    f = 'streamTest.delMe'
    ret = stream(f, 'C', "OPEN")
    say 'ret:' ret '?' (ret == "READY:")

    [miesfeld@Osprey ooTest]$ rexx qTest.rex
    Before realpath() name=/home/miesfeld/work.ooRexx/ooTest/rexx.img tempName=
    After realpath() temp=(nil) name=/home/miesfeld/work.ooRexx/ooTest/rexx.img tempName=/home/miesfeld/work.ooRexx/ooTest/rexx.img
    Before realpath() name=/home/miesfeld/work.ooRexx/ooTest/rexx.img tempName=
    After realpath() temp=(nil) name=/home/miesfeld/work.ooRexx/ooTest/rexx.img tempName=/home/miesfeld/work.ooRexx/ooTest/rexx.img
    Before realpath() name=/home/miesfeld/work.ooRexx/ooTest/framework/rexx.img tempName=
    After realpath() temp=(nil) name=/home/miesfeld/work.ooRexx/ooTest/framework/rexx.img tempName=/home/miesfeld/work.ooRexx/ooTest/framework/rexx.img
    Before realpath() name=/opt/slickedit-14.0.1.2/bin/rexx.img tempName=
    After realpath() temp=(nil) name=/opt/slickedit-14.0.1.2/bin/rexx.img tempName=/opt/slickedit-14.0.1.2/bin/rexx.img
    Before realpath() name=/tools/dfxRexxLib/rexx.img tempName=
    After realpath() temp=(nil) name=/tools/dfxRexxLib/rexx.img tempName=/tools
    Before realpath() name=/usr/lib/qt-3.3/bin/rexx.img tempName=
    After realpath() temp=(nil) name=/usr/lib/qt-3.3/bin/rexx.img tempName=/usr/lib/qt-3.3/bin/rexx.img
    Before realpath() name=/usr/kerberos/bin/rexx.img tempName=
    After realpath() temp=(nil) name=/usr/kerberos/bin/rexx.img tempName=/usr/kerberos/bin/rexx.img
    Before realpath() name=/usr/lib/ccache/rexx.img tempName=
    After realpath() temp=(nil) name=/usr/lib/ccache/rexx.img tempName=/usr/lib/ccache/rexx.img
    Before realpath() name=/usr/local/bin/rexx.img tempName=
    After realpath() temp=(nil) name=/usr/local/bin/rexx.img tempName=/usr/local/bin/rexx.img
    Before realpath() name=/usr/bin/rexx.img tempName=
    After realpath() temp=0xbff546a8 name=/usr/bin/rexx.img tempName=/opt/ooRexx/bin/rexx.img
    In stream_open()
    In stream_open()
    In stream_open()
    Before realpath() name=/home/miesfeld/work.ooRexx/ooTest/qTest.rex tempName=
    After realpath() temp=0xbff546e8 name=/home/miesfeld/work.ooRexx/ooTest/qTest.rex tempName=/home/miesfeld/work.ooRexx/ooTest/qTest.rex
    Before realpath() name=/home/miesfeld/work.ooRexx/ooTest/streamTest.delMe tempName=
    After realpath() temp=(nil) name=/home/miesfeld/work.ooRexx/ooTest/streamTest.delMe tempName=/home/miesfeld/work.ooRexx/ooTest/streamTest.delMe
    qualifyStreamName() done=0 tempPath=/home/miesfeld/work.ooRexx/ooTest/streamTest.delMe
    In stream_open()
    Before realpath() name=/home/miesfeld/work.ooRexx/ooTest/streamTest.delMe tempName=
    After realpath() temp=(nil) name=/home/miesfeld/work.ooRexx/ooTest/streamTest.delMe tempName=/home/miesfeld/work.ooRexx/ooTest/streamTest.delMe
    qualifyStreamName() done=0 tempPath=/home/miesfeld/work.ooRexx/ooTest/streamTest.delMe
    ret: ERROR:2 ? 0

     
  • jfaucher
    jfaucher
    2009-07-21

    Mark,

    Yes, when I tested, I did not think to test with a non existing file :-)
    The Unix command realpath has the same behavior, error if non existent file.

    I see that realpath() not only removes "." and ".." but also resolve symbolic links.
    Do we need to resolve the symbolic links ?
    I don't see anything written about that in the ooRexx doc.
    If no, then we can write our own function which removes the "." and "..".
    Will work even if any part of the path does not exist.
    And probably safer than using the result of realpath() despite the error code :-)
    If you agree and not done before next WE, then I will have a look at that.
    Could be an additional parameter passed to SysFileSystem::canonicalizeName to indicate if realpath() must be used or not.

    Jean-Louis

     
  • Mark Miesfeld
    Mark Miesfeld
    2009-07-21

    Committed revision 4969.

    Jean-Louis is right about it being safer to have our own function to normalize the path name. The man page says that if realpath() returns NULL, the resolved path is undefined. So, even if it seems to be okay on Linux, who knows what it does on other operating systems.

    I replaced realpath() with our own function. All the test cases pass, including the new relative path tests. The relative path test cases all fail under the beta build.

    I think this is fixed now, so I'll probably put the fix in the beta branch shortly.

     


Anonymous


Cancel   Add attachments