Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sucessful calls might return an error value #7

Open
StyXman opened this issue Nov 12, 2019 · 5 comments
Open

Sucessful calls might return an error value #7

StyXman opened this issue Nov 12, 2019 · 5 comments

Comments

@StyXman
Copy link

StyXman commented Nov 12, 2019

According to the docs:

Any C function (even void functions) may be called in a multiple assignment context to retrieve both the return value (if any) and the C errno variable as an error

But crypt() does not set errno, and the call to C.crypt() is building a error variable with a value of some other call. If errno is not 0, then err is not nil. Both

https://github.com/amoghe/go-crypt/blob/master/crypt.go#L41

and

https://github.com/amoghe/go-crypt/blob/master/crypt.go#L48

are returning an unrelated value.

So this code might or might fail:

	password, err := Crypt(password, salt)
	if err != nil {
		// TODO: oh no!
		log.Printf("crypt: [%v] %v", password, err)
		return err
	}

Like this:

2019/11/12 05:56:42 $6$iq.dbF2KUB2h, x
2019/11/12 05:56:42 crypt: [$6$iq.dbF2KUB2h$nV.xPcJUqHN1r46FQ/MJJc7OVbSb5Yd4nsOLU.XZ3QsVdwgGjqVoNeEx.u8MIFC1seQcS8jAelI95Z8BlBmeC/] errno -8191
@StyXman
Copy link
Author

StyXman commented Nov 12, 2019

Hmmm, hold that thought, crypt() is supposed to set errno, hmmm....

@amoghe
Copy link
Owner

amoghe commented Nov 12, 2019

What I gleaned from the documentation is that:

  1. if crypt (or any other libc call) returns with no exitcode, then do NOT look at errno, it may not be cleared and may still hold the last error
  2. if crypt returns an error, then errno will be set

and thats how this library behaves w.r.t errors from libc

@StyXman
Copy link
Author

StyXman commented Nov 12, 2019

Yeah, but that breaks the golang way. The usual thing to do when wrapping for a language is to follow the patterns of that language, otherwise anyone using it finds it surprising, like in this case. Of course, it's your code, so you're free to decide that. Just try to add a note somewhere that it was a conscious decision. Maybe just point to this issue and close it.

@amoghe
Copy link
Owner

amoghe commented Nov 12, 2019

Do you have any thoughts/suggestions on how to structure the code better to avoid this confusion?

@StyXman
Copy link
Author

StyXman commented Nov 13, 2019

Well, golang's pattern here is that you do:

result, err := call(params)
if err != nil {
    // handle error
}

So just

	return C.GoString(c_enc), nil

in line 48.

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

No branches or pull requests

2 participants