Skip to content

Conversation

musm
Copy link
Contributor

@musm musm commented Jun 11, 2019

No description provided.

skip, stat, unsafe_read, unsafe_write, write, transcode, uv_error,
rawhandle, OS_HANDLE, INVALID_OS_HANDLE, windowserror

import .Base.RefValue
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the import in the module file, since it's used by both path.jl and file.jl (it was indirectly called in path.jl even though the import is in file.jl) this should make the intent clear.

@ararslan ararslan requested review from vtjnash and stevengj June 12, 2019 14:44
@StefanKarpinski
Copy link
Member

@vtjnash, can you take a quick look at this and merge if it looks good to you. I had a glance through and it looked reasonable to me.

@stevengj
Copy link
Member

stevengj commented Jun 13, 2019

I see, since StringVector(n) allocates n+1 bytes (i.e. it includes space for the trailing NUL), then I guess we can use path_max both here and in #31434 (with a comment in the source), with length(buf)+1 for the size.

@musm
Copy link
Contributor Author

musm commented Jun 13, 2019

@stevengj is there any advantage to the suggestion of using StringVector here?
pathmax is the max buffer size including the null so the code actually should look like

function pwd()
    path_max = 1024
    buf = Base.StringVector(path_max - 1) # space for null-terminator implied by StringVector
    sz = RefValue{Csize_t}(length(buf) + 1)
    while true
        rc = ccall(:uv_cwd, Cint, (Ptr{UInt8}, Ptr{Csize_t}), buf, sz)
        if rc == 0
            resize!(buf, sz[])
            return String(buf)
        elseif rc == Base.UV_ENOBUFS
            resize!(buf, sz[] - 1) # space for null-terminator is implied by String
        else
            uv_error(:cwd, rc)
        end
    end
end

vs.

function pwd()
    path_max = 1024
    buf = Vector{UInt8}(undef, path_max) 
    sz = RefValue{Csize_t}(length(buf))
    while true
        rc = ccall(:uv_cwd, Cint, (Ptr{UInt8}, Ptr{Csize_t}), buf, sz)
        if rc == 0
            resize!(buf, sz[])
            return String(buf)
        elseif rc == Base.UV_ENOBUFS
            resize!(buf, sz[]) 
        else
            uv_error(:cwd, rc)
        end
    end
end

@musm
Copy link
Contributor Author

musm commented Jun 13, 2019

pathmax=1024 is also too large for windows, especially with the resize. But these can be audited in a different PR.

@stevengj
Copy link
Member

is there any advantage to the suggestion of using StringVector here?

A StringVector can be converted to a String without making a copy. A generic Vector{UInt8} cannot.

@musm musm force-pushed the homedir branch 3 times, most recently from 29f51b7 to 6dde899 Compare June 16, 2019 16:51
@musm
Copy link
Contributor Author

musm commented Jun 16, 2019

I'm happy with the PR now and is ready on my end, just needs a final review.

@musm
Copy link
Contributor Author

musm commented Jun 18, 2019

thanks for the review @stevengj
@vtjnash does this look ok with you? Can we merge?

import .Base: cwstring
end

const MAX_PATH = Sys.iswindows() ? 260 : 1024 # max buffer size including null terminator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we perhaps give this a more accurate name? The real MAX_PATH on NT is 32768, on Win32 is 260, on bsd sockets is 108, on linux is 4096, etc.

Since we have logic to resize as needed, perhaps we should call this AVG_PATH and define it to always be some moderate sized value (like 108 or 260)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'm not sure on which value makes the most sense on average. I think the current numbers seem quite reasonable on average for non-Windows systems (we could go with 512)?
For windows it seems reasonable however to just use 260.

Renaming the constant makes sense I believe.

elseif rc == Base.UV_ENOBUFS
resize!(buf, sz[] - 1) # space for null-terminator implied by StringVector
else
uv_error(:cwd, rc)
Copy link
Member

@vtjnash vtjnash Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we call this "pwd", to reflect the name of the Julia function, or keep the name "getcwd", to reflect the name of the libc function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I called it cwd to reflect the name of the libuv call, which I decided was perhaps more helpful for debugging purposes. If the pwd() function errors in julia it will be clear which function is throwing the error from the stacktrace, but perhaps not clear which internal function caused the error and thus I decided to call it :cwd instead of pwd.

@musm
Copy link
Contributor Author

musm commented Jun 18, 2019

Feedback addressed: I decided to go with 512 buffer length on non-windows systems. The value 1024 seems a little too long for average file system operations. On Windows I think the only value that makes sense is 260

@musm
Copy link
Contributor Author

musm commented Jun 18, 2019

Win32: timeout
Win64: unable to access 'https://github.com/JuliaLang/julia/': Could not resolve host: github.com
Win64 Appveyor: Error in testset Profile:
macos64: : Error in FileWatching

Rebased to master by force pushing to re-kick CI.

@musm musm changed the title Simplify homedir logic and pwd functions Update homedir and pwd to use StringVector and buffer resizing Jun 18, 2019
@musm
Copy link
Contributor Author

musm commented Jun 19, 2019

unrelated download failure on win64

@stevengj stevengj merged commit d309635 into JuliaLang:master Jun 19, 2019
@musm musm deleted the homedir branch June 19, 2019 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants