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

Unused code gets go vet error #7

Open
spacewander opened this issue Oct 22, 2022 · 3 comments
Open

Unused code gets go vet error #7

spacewander opened this issue Oct 22, 2022 · 3 comments

Comments

@spacewander
Copy link
Member

 go vet ./...
# mosn.io/envoy-go-extension/pkg/utils
pkg/utils/string.go:12:22: possible misuse of unsafe.Pointer
pkg/utils/string.go:20:22: possible misuse of unsafe.Pointer
# mosn.io/envoy-go-extension/pkg/http
pkg/http/capi.go:36:79: possibly passing Go type with embedded pointer to C

I have checked all the errors, and it seems they come from unused file or argument, so we can simply remove them.

	C.moeHttpSendLocalReply(r, C.int(response_code), unsafe.Pointer(&body_text), unsafe.Pointer(&strs), C.longlong(grpc_status), unsafe.Pointer(&details))

We can pass the addr of a flat buffer instead of []string to avoid possibly passing Go type with embedded pointer to C

	sHdr.Data = uintptr(unsafe.Pointer(uintptr(ptr)))

It seems we can simply write sHdr.Data = uintptr(ptr) in this case? Do I miss something?

@doujiang24
Copy link
Member

@spacewander Nice catch~

  1. for possible misuse of unsafe.Pointer
    yes, it's a misuse. Patch welcome ~

  2. for possibly passing Go type with embedded pointer to C
    it's an expected warning. It's unsafe usage for cgo, which is also desired.
    (We also disabled cgocheck for unsafe passing Go pointer from go to c and c to go).

We can pass the addr of a flat buffer instead of []string

this could be a proper way to mute this vet warning, but I'm not sure if it's worthing, it may complicate the code.
we need to reflect the flat buffer into []string before using it as strings. thoughts?

@spacewander
Copy link
Member Author

this could be a proper way to mute this vet warning, but I'm not sure if it's worthing, it may complicate the code.

What about passing the Data ptr like:

strs := make([]string, num*2)
// but, this buffer can not be reused safely,
// since strings may refer to this buffer as string data, and string is const in go.
// we have to make sure the all strings is not using before reusing,
// but strings may be alive beyond the request life.
buf := make([]byte, bytes)
sHeader := (*reflect.SliceHeader)(unsafe.Pointer(&strs))
bHeader := (*reflect.SliceHeader)(unsafe.Pointer(&buf))
C.moeHttpCopyHeaders(r, unsafe.Pointer(sHeader.Data), unsafe.Pointer(bHeader.Data))

spacewander added a commit to spacewander/envoy-go-extension that referenced this issue Oct 25, 2022
@spacewander
Copy link
Member Author

Actually we can delay the possibly passing Go type with embedded pointer to C suppression as the headers argument is not used at all. 😃

doujiang24 pushed a commit that referenced this issue Oct 26, 2022
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