Can't determine if dir already exists with Mkdir

[purpleidea]

I'd like to determine if after running a mkdir if a failure is due to the directory already existing. I've done:

if status, ok := err.(*sftp.StatusError); ok {
    log.Printf("Code: %v, %v", status.Code, status.Error())
    if status.Code == ??? {
        XXX
    }
}

but unfortunately there doesn't seem to be enough information in StatusError or the Code to determine this. Can it be added?

Thanks!

[davecheney]

I wonder if we could reuse os.PathErr here, then maybe the os.IsExist
helper could be bought into play.

On Sat, Aug 6, 2016 at 8:55 PM, James notifications@github.com wrote:

I'd like to determine if after running a mkdir if a failure is due to the
directory already existing. I've done:

if status, ok := err.(*sftp.StatusError); ok {
log.Printf("Code: %v, %v", status.Code, status.Error())
if status.Code == ??? {
XXX
}
}

but unfortunately there doesn't seem to be enough information in
StatusError or the Code to determine this. Can it be added?

Thanks!

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#131, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAcA58a1A1a3WT1YsVgG5zzvqUobix8ks5qdGgagaJpZM4JeQaR
.

[eikenb]

I might take a look at this once I've finished reviewing everything else and assuming I have the time. If you could provide self contained, small example code that replicated the issue that would be a great help. If you have an idea how to fix it yourself and would like to submit the fix+test in a pull request that would be even better.

Thanks.

[purpleidea]

If you could provide self contained, small example code that replicated the issue

@eikenb ??? That's what's in my first comment describing the issue.

[eikenb]

I meant self-contained meaning I could just cut-n-paste and run it, or as close to that as possible. The code snippet is helpful in expressing what you'd like to see but still requires time to write the code to try out your idea. And the less time it takes to try/test/implement an idea the greater the chance that I'll be able to get to it.

[eikenb]

No worries anymore. While working on the other client issues I've written enough client code to realize this is trivial to reproduce.

Doing some comparison testing between openssh's sftp client and ours they actually have similar behaviors for mkdir errors. When the directory already exists they both return a simple "Failure" message. When the directory can't be created due to the parent directory not existing, they both return a "No such file" type error.

What other cases do you get the Failure message that you can't discern from Mkdir? The only other one that comes to mind at the moment is if a file already exists of that name instead of a directory.

[eikenb]

When the named path already exists as a file and you try mkdir, the openssh sftp client returns a simple Failure. So that case is the same as well. It seems the servers don't return enough information to tell the difference between these 2 situations.

[eikenb]

The only way I can think to do this is to have the client run a stat on the path after getting the "Failure" reply. The stat data includes enough data to check if it is directory. It would complicate things and add a second call in case of errors. This is something that could just as easily be done by the app using the client code, so I'm not 100% it should be done in the client lib. I'll think about it.

[binary132]

I'm not sure whether this is directly related, but Client.ReadDir appears to only use normaliseError in case of an error returned from sendPacket; Client.openDir does not normalize its error (https://github.com/pkg/sftp/blob/master/client.go#L206). Is this an intended behavior?

normaliseError [sic 😜] returns os.ErrNotExist in case of missing files. I suspect this is just a bug since normaliseError is called on nearly all other similar cases.

[eikenb]

Based on the commit, it looks like they just missed that one...

3fa7dab

[eikenb]

The returned unmarshalStatus() values that don't go through normaliseError() have no bearing on the Mkdir() return values, but it does seem inconsistent that all but 3 of the unmarshalStatus() returns are not wrapped. I'm going to add the wrapper so all the uses are consistent.

[purpleidea]

Hey @eikenb could you either update us on your status with this issue or un-self-assign if you're not working on this anymore? Thanks!

[eikenb]

@purpleidea I'm thinking about just closing it. In my comment back on Feb 16th I talk about how this could possibly be implemented and how I wasn't sure if it was something that belonged in the library or not. I'm currently leaning toward taking both this ticket and #144 and adding documentation and/or examples on how to handle these situations.

[purpleidea]

@eikenb I don't think it's a documentation issue, it just needs to return the right error. eg: https://github.com/purpleidea/mgmt/blob/master/remote/remote.go#L203

[eikenb]

I gave this one last look and what you want is just not possible. I looked at openssh's sftp server mkdir implementation and it doesn't return the information needed to enable the client to know if the error is due to a directory already existing with the same name. In the case that a the directory already exists it returns a general SSH2_FX_FAILURE. It returns this same error in all these error cases...

EDQUOT - quota limit hit
EEXIST - file, directory, sym-link already exists with that name
EMLINK - parent directory has to many files
ENOMEM - out of memory error
ENOSPC - no space on device
EROFS - read-only file system error

I got this by comparing all the possible errors returned by the mkdir system call to what the sftp server handles and which SFTP status packets it sends for each error.

So the only way to do what you want is to check the returned status and if it contains the error-code SSH2_FX_FAILURE (type uint32, value 4) then do a client.Stat() call on that path and check the returned os.FileInfo value to see if it is a directory.

This is why I think this could be a documentation issue, because that pattern could be documented and possibly put in an example.

[purpleidea]

@eikenb I'm not a specialist on the protocol level, maybe @davecheney has some thoughts?

I think your idea about the Stat after failure is great. It makes sense since this is a wrapper library too. Good thinking!

[binary132]

Thanks for the investigation @eikenb. Good breakdown. Perhaps if such information is required, the user ought to begin with the stat.

[eikenb]

I'm just going to add an example in the documentation for this. It will show up as an example under Mkdir. I'm readying the commit.

[purpleidea]

@eikenb Did you give up on your stat after failure idea?

[eikenb]

The Mkdir() method as it is currently implemented closely follows the protocol and the openssh implementation (which I consider to be the standard/reference implementation). I think this is generally a good thing and would like to keep it as is.

My idea for using Stat() was that it would be easy for a developer using the library to do the Stat() check themselves and handle things as they deem appropriate. That was why I thought putting it in an example best addressed the issue.

Once a "1.0" version is done I'll be open to ideas about adding a layer of abstraction on top of the existing code as a more managed/high-level client experience.